Skip to content
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

Asynchronous Server listening #60

Merged
merged 1 commit into from
Jan 12, 2019
Merged

Conversation

joriscarrier
Copy link
Contributor

No description provided.

@bmuller
Copy link
Owner

bmuller commented Jan 11, 2019

Thanks @joriscarrier! The only thing that I'm still unsure about is whether or not it makes sense to allow passing in an explicit loop in the Server constructor. Is there a reason someone would want to pass that in rather than just calling asyncio.set_event_loop()?

@joriscarrier
Copy link
Contributor Author

joriscarrier commented Jan 11, 2019

@bmuller yes, after reading several articles it is better to pass the event loop to all functions than to call asyncio.get_event_loop: if you call asyncio.get_event_loop() from within a coroutine you might not get the event loop back that ran you and also typically when you add non-trivial tests,

https://vorpus.org/blog/some-thoughts-on-asynchronous-api-design-in-a-post-asyncawait-world/

@bmuller
Copy link
Owner

bmuller commented Jan 11, 2019

@joriscarrier I may be reading this wrong, but I think the recent edit in that post seems to indicate get_event_loop should consistently return the correct loop:

[Edit: I may have been overly pessimistic! I'm told that asyncio's global event loop fetching API is going to be reworked in 3.6 and backported to 3.5.3. If I understand correctly (which is not 100% certain, and I don't think the actual code has been written yet [edit**2: here it is]), the new system will be: asyncio.get_event_loop(), instead of directly calling the currently-registered AbstractEventLoopPolicy's get_event_loop() method, will first check some thread-local global to see if a Task is currently executing, and if so it will immediately return the event loop associated with that Task (and otherwise it will continue to fall back on the AbstractEventLoopPolicy. This means that inside async functions it should now be guaranteed (via somewhat indirect means) that asyncio.get_event_loop() gives you the same event loop that you'd get by doing an await. And, more importantly, since asyncio.get_event_loop() is what the callback-level APIs use to pick a default event loop when one isn't specified, this also means that async/await code should be able to safely use callback-layer functions without explicitly specifying an event loop, which is a neat improvement over my suggestion above.

@joriscarrier
Copy link
Contributor Author

@bmuller do you want me to work again to reuse asyncio.get_event_loop() it's not a problem for me?

@bmuller
Copy link
Owner

bmuller commented Jan 11, 2019

Hey @joriscarrier - Yeah, I think just for simplicity for now we could stick to using asyncio.get_event_loop(). The rest looks great though! Thanks so much for the help.

@joriscarrier
Copy link
Contributor Author

@bmuller I did, I also move the test folder to the root of the project and add the code coverage, if anything bothers you I would be happy to help you

@bmuller
Copy link
Owner

bmuller commented Jan 12, 2019

Hey @joriscarrier - this PR has moved substantially beyond the purpose in the title - "Asynchronous Server listening". In general - it's much easier for to review PR's with a single focus, rather than having a massive merge request incorporating tons of changes relating to lots of different things. For instance - the async server listening is something I'd merge now - and the use of abstractmethods in the interface - but some of the testing changes are things I'd want to get some more clarity on (and if those were in a different PR we could discuss those separately and independently).

@joriscarrier
Copy link
Contributor Author

@bmuller I did

@bmuller bmuller merged commit be6b608 into bmuller:master Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants