Removed explicit 'Nursery' from Listener

[aratz-lasa]

What was wrong?

The IListener interface is tightly coupled to the I/O implementation (Trio). This makes the application not to be agnostic to the I/O implementation.

Even if it goes quite against Trio's Nursery philosophy, we must decide whether we prioritize the abstractions and I/O agnostic implementations, or to the contrary, Trio's structured concurrent philosophy.
In order to understand better trio's Philosophy you can check: Trio's philosphy

How was it fixed?

Created a static class TrioGlobalNursery with two methods:

1. set_global_nursery_and_run: which sets the specified nursery as 'Global' and runs an asynchronous function, so that if you later before the function returns get_global_nursery, it returns the 'Global Nursery'
2. get_global_nursery: returns the current 'Global Nursery'

This way, for running the IListener listen you should do it by using:

await TrioGlobalNursery.set_global_nursery_and_run(nursery, listener.listen, maddr)

Instead of:

await listener.listen(maddr, nursery)

To-Do

• Check why py37-interop fails

[mhchia]

@aratz-lasa No worries about the failure of py37-interop. I overwrote the branch test/add-options of my cloned repo mhchia/go-libp2p-daemon, which is needed by the CI test. This branch depends on a library that only exists in go==1.13. I am fixing it in #406. If needed, I can cherrypick the commits to another PR to make the CI happy.

[aratz-lasa]

@mhchia Okay, thank you!
Should I ping someone in order to discuss this Trio Nursery work-around?

[pipermerriam]

I'm 👎

It accomplishes removal of the trio specific nursery argument for global application state which in my opinion simply hides the problem behind a common antipattern. The Listener is still coupled with the trio nursery, only now it is hidden via a global embedded within the class.

I don't think this improves the current state of things.

[ralexstokes]

thanks for this PR!

i agree we should remove the reference to the trio.Nursery from the IListener interface.

but given how lightweight trio nurseries are, we can just do something like

class TrioTCPListener(IListener):
    async def listen(self, maddr):
        async with trio.open_nursery() as nursery:
            # do stuff to start the actions of this listener
            ...

if we do in fact need to control the nursery, then the instantiator of TrioTCPListener can provide one in the __init__.

class TrioTCPListener(IListener):
    def __init__(self, nursery):
        # we can use this nursery inside e.g. `listen` if we need to use a specific nursery
        self._nursery = nursery

it seems to be adding an extra level of indirection to introduce the TrioGlobalNursery when we can just take advantage of hiding these implementation details behind our nice IListener interface...

[aratz-lasa]

@ralexstokes You are completely right and that was my first thought, however, we need to control the nursery because it is the one used in the Swarm. Also there is no possibility to put the Nursery on the __init__ because in order to create a Listener we use transport.create_listener(conn_handler), instead of directly creating it.

[aratz-lasa]

@pipermerriam I understand it, and I am not sure if this the best way to go. I have made a pull request to discuss about it

[pipermerriam]

@aratz-lasa apologies if my tone came across negative. I do appreciate the PR and your contribution so not trying to shut you down. I'm very open to considering alternatives.

Short term, I'm personally fine with the trio dependency but that is primarily because transitioning to trio is inline with the use case I specifically have for this library and I want to get it off the ground and working in the wild with minimal delays. Medium/long term, I'm very game for this library to move towards being agnostic to asyncio/trio.

[aratz-lasa]

@aratz-lasa apologies if my tone came across negative. I do appreciate the PR and your contribution so not trying to shut you down. I'm very open to considering alternatives.

Short term, I'm personally fine with the trio dependency but that is primarily because transitioning to trio is inline with the use case I specifically have for this library and I want to get it off the ground and working in the wild with minimal delays. Medium/long term, I'm very game for this library to move towards being agnostic to asyncio/trio.

@pipermerriam Okay, okay, I understand you. I think I have an alternative solution you may like: what if for now, we change the nursery argument on ILIstener by task_status. This way, listen can be called by nursery.start. And in consequence, on the one hand, there is no nursery explicit passing as argument, and on the other hand, it is an optional argument where you cannot use it if you don't want to.

I have changed the commit, so that it implements task_status.

[ralexstokes]

@aratz-lasa

Also there is no possibility to put the Nursery on the init because in order to create a Listener we use transport.create_listener(conn_handler), instead of directly creating it.

why can't we just directly create the TCPListener?

[aratz-lasa]

@ralexstokes

why can't we just directly create the TCPListener?

Well, even if right now is only implemented TCP, the Swarm must be Transport layer agnostic, that is why we invoke transport.create_listener(conn_handler), instead of directly creating a TCPListener.

Or at least that is what I understood reading the source code, I may be wrong. I am still quite new to this project :)

[ralexstokes]

@aratz-lasa so this quickly becomes a bigger question about API and the higher-order structure of this library but if we want to remove concrete IO from our abstract interfaces (i think a worthy goal), then a straightforward way to do this would be to have create_listener return some opaque Task type (like in async-service, e.g. https://github.com/ethereum/async-service/blob/master/async_service/base.py#L100) which is the passed up to outer layers of the libp2p library until it hits a "task runner", aka a thing that can drive the concurrency here... this was kind of the thinking that lead to the discussion in this issue: #349

right now a Swarm (via inheriting async_service.Service) is the "task runner" so it seems clear how we can deploy this strategy here...

[ralexstokes]

it also isn't immediately clear to me we need a separate listener_nursery but this is likely a different conversation :)

the fewer places we can "effect" the concurrency the simpler time we will have debugging/maintaining/ etc

[aratz-lasa]

@ralexstokes I think you are right, do I commit a version with create_listener returning a Task? Or do we leave it for now?
If we leave it, I would recommend changing nursery by task_status from IListener interface, as in my last commit on this pull request.

[aratz-lasa]

it also isn't immediately clear to me we need a separate listener_nursery but this is likely a different conversation :)

the fewer places we can "effect" the concurrency the simpler time we will have debugging/maintaining/ etc

Yeah, I also thought about it. We should have a clear strategy for when to use Nursery, and what is every nursery's scope. Because depending on the strategy, as you said, maybe there is no need for having a nursery on the Swarm.