-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed explicit 'Nursery' from Listener #407
base: main
Are you sure you want to change the base?
Conversation
@aratz-lasa No worries about the failure of |
@mhchia Okay, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@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 |
@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 |
@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. |
795d53f
to
f8d32a0
Compare
@pipermerriam Okay, okay, I understand you. I think I have an alternative solution you may like: what if for now, we change the I have changed the commit, so that it implements |
f8d32a0
to
d19fa4d
Compare
This way it is possible to implement a I/O agnostic Listener. It would still have the 'task_status' default argument, but there would be no need for receiving it. And this way there is also no need for explicit nursery passing
d19fa4d
to
b53675e
Compare
why can't we just directly create the |
Well, even if right now is only implemented TCP, the Or at least that is what I understood reading the source code, I may be wrong. I am still quite new to this project :) |
@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 right now a |
it also isn't immediately clear to me we need a separate the fewer places we can "effect" the concurrency the simpler time we will have debugging/maintaining/ etc |
@ralexstokes I think you are right, do I commit a version with |
Yeah, I also thought about it. We should have a clear strategy for when to use |
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:
nursery
as 'Global' and runs an asynchronous function, so that if you later before the function returnsget_global_nursery
, it returns the '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