Open 1st1 opened 7 years ago
If my voice counts, I am definitely +1 on having run
in 3.6
+1, definitely!
About asyncio.forever()
, wouldn't it be simpler to have a wrapper around loop.add_signal_handler(SIGINT, ...)
? For instance:
async def wait_for_interrupt():
loop = asyncio.get_event_loop()
future = loop.create_future()
loop.add_signal_handler(signal.SIGINT, future.set_result, None)
try:
await future
finally:
loop.remove_signal_handler(signal.SIGINT)
+1, LGTM Do we need tests? I pretty sure we do.
About asyncio.forever(), wouldn't it be simpler to have a wrapper around loop.add_signal_handler(SIGINT, ...)? For instance:
This would work to support signals. I want asyncio.forever()
to support try..finally
blocks regardless of what stopped the loop.
try:
await asyncio.forever()
finally:
# important cleanup code code
TBH I don't want to distract ourselves with asyncio.forever()
design. That stuff will require a separate discussion.
Do we need tests? I pretty sure we do.
For sure. As I explained in the first message, if Guido is in favour of the idea I'll add tests/docs/etc.
Sure, add this. I don't have time for a review though.
Sure, add this. I don't have time for a review though.
I've experimented a little bit, and it turns out that it's not that hard to implement the asyncio.forever()
coroutine. The idea is to add a Future object that loop.run_forever()
creates and sets the result to before returning.
With the latest commit it's possible to write coroutines like this:
async def foo():
print('hi')
try:
await asyncio.forever()
except KeyboardInterrupt:
await asyncio.sleep(1)
print('bye')
asyncio.run(foo())
The change modifies Task._wakeup
to handle BaseException
(that is a safe and backwards compatible thing to do), and adds a new low-level API method to the loop: get_forever_future()
.
So forever is just sleep infinity? Why do we need that?
--Guido (mobile)
So forever is just sleep infinity? Why do we need that?
It's similar but not the same. It's designed to replace uses of loop.run_forever()
, moving the program cleanup logic into the main coroutine.
It allows to safely transform this:
loop = asyncio.get_event_loop()
server = loop.run_until_complete(
asyncio.start_server(handle_echo, '127.0.0.1', 8888, loop=loop))
try:
loop.run_forever()
except KeyboardInterrupt:
pass
# Close the server
server.close()
loop.run_until_complete(server.wait_closed())
loop.close()
into this:
async def main():
server = await asyncio.start_server(handle_echo, '127.0.0.1', 8888)
try:
await asyncio.forever()
except KeyboardInterrupt:
pass
server.close()
await server.wait_closed()
asyncio.run(main())
The former example that uses run_forever
and run_until_complete
is incomplete, btw. It doesn't use try..finally
to guarantee the loop.close()
call, and it doesn't shutdown generators. The latter example that uses asyncio.run()
does all of that.
The key difference from 'sleep(inf)is that you can use
try..except..finallyaround
await forever()to properly cleanup when
KeyboardInterrupt` occurs or loop is stopped.
Having all program bootstrap logic defined in one coroutine is easier than using loop.run_until_complete()
, passing the loop around, calling loop.run_forever
, and later cleaning up the loop.
asyncio.run()
would be useful even without asyncio.forever()
. If you only open connections or spawn subrocesses, you can use it. But if you have a server, or a number of other coroutines that run in "background", you'd need something to await on until the program is done.
If you don't see any pitfalls with asyncio.forever()
then I think we have a huge win here. Essentially, event loop then becomes a low-level API that will be recommended for "expert" users. We won't even need to bring it up in asyncio tutorials/examples.
-1. Something that's only meant to be used at the top level doesn't deserve to be a coroutine IMO.
And even if we have forever() I think you should be able to get the same behavior (relative to KeyboardInterrupt) with sleep(100000).
A few comments about asyncio.run()
:
asyncio.run()
to the main thread a bit extreme? I guess it might help avoiding some issues though, e.g subprocess handling on unix systems.Shouldn't the current loop be saved and restored in the finally
clause? The current implementation can lead to surprising results:
loop = asyncio.get_event_loop() # Fine!
asyncio.run(some_coro()) # Run coroutine
loop = asyncio.get_event_loop() # Raise a RuntimeError
@1st1
Like you said earlier, asyncio.forever()
shouldn't distract us from getting asyncio.run
merged, so I'll happily move my comments if you happen to make another PR/issue specially for asyncio.forever()
. Anyway, here are my two cents:
I'm not sure it makes sense to have an asynchronous equivalent to loop.run_forever
.
loop.run_forever
: run until the loop is stoppedawait asyncio.forever
: wait until the loop is stoppedBut if the loop is stopped, how can the coroutine keep running? In the current implementation, it is restored by running the loop a second time. So asyncio.forever()
actually means "wait until the loop is restarted", which is a bit confusing in my opinion. That also means loop.stop()
might be used to notify asyncio.forever()
, which is also quite weird.
I would argue that a wait_for_interrupt()
coroutine is actually enough to cover most of the loop.run_forever()
use case. When asyncio.run()
is used, it should be safe to assume that the loop doesn't stop until the main coroutine is completed.
@gvanrossum
-1. Something that's only meant to be used at the top level doesn't deserve to be a coroutine IMO.
Normally yes, I agree. Although I don't think this argument fully applies to this particular use case.
The idea is to add APIs to make it possible to move the application bootstrap logic (the "main" function") into a coroutine. The design of asyncio.start_server()
and loop.create_server()
API requires you to use loop.run_forever()
function to properly cleanup.
asyncio.run()
solves a lot of cases, but without something like asyncio.forever()
it cannot help you when your application starts a server. And servers might be the majority of asyncio programs out there. Please take a look at the code example a couple of paragraphs below.
And even if we have forever() I think you should be able to get the same behavior (relative to KeyboardInterrupt) with sleep(100000).
I see your point. Unfortunately it's not possible to implement this behaviour in asyncio.sleep
, so we'd definitely need a better name for forever()
.
What if we rename forever()
to interrupted()
:
async def main():
server = await asyncio.start_server(handle_echo, '127.0.0.1', 8888)
try:
await asyncio.interrupted()
finally:
server.close()
await server.wait_closed()
asyncio.run(main())
@vxgmichel
- Isn't restricting asyncio.run() to the main thread a bit extreme? I guess it might help avoiding some issues though, e.g subprocess handling on unix systems.
The function is supposed be used to launch your main program coroutine and we recommend to use asyncio in the main thread. And yes, subprocesses don't properly work when the loop isn't running in the main thread. I don't think that lifting this restriction would help anyone to be honest.
- About the clean-up, would it make sense to run the loop once, to get rid of possible pending callbacks? I remember I had to do that a few times to avoid some warnings.
Well, we run loop.shutdown_asyncgens()
before closing, isn't that enough? I don't want to add another props for that, as it's just masking bugs and lack of some APIs in asyncio.
- Shouldn't the current loop be saved and restored in the finally clause? The current implementation can lead to surprising results:
Can't do that. asyncio.get_event_loop
behaves in weird ways with the default policy:
RuntimeError(There is no current event loop in thread)
after first call to asyncio.set_event_loop(None)
.asyncio.get_event_loop
returns is new or it was used before. Restoring a loop that was just created doesn't make any sense.I'm -1 on all three.
I'm not sure it makes sense to have an asynchronous equivalent to loop.run_forever.
- loop.run_forever: run until the loop is stopped
- await asyncio.forever: wait until the loop is stopped But if the loop is stopped, how can the coroutine keep running?
It's up to the documentation -- the idea is that forever()
wakes up when the loop is interrupted by a BaseException
or was stopped by loop.stop()
. It then allows you to cleanup your resources. wait_for_interrupt()
doesn't cover that -- it only covers KeyboardInterrupt
.
Please study my example code in this comment and in https://github.com/python/asyncio/pull/465#issuecomment-260521057.
+1 for keeping .run()
only for main thread.
Also I would like to have .interrupt()
. @1st1 has described the need very well I believe.
Yep, forever
was ambiguous name but interrupt()
sounds very clean for me.
That's also ambiguous (is it a verb or a noun?). And the issue of whether it would just be a shorthand for sleep(inf) is still unsettled.
That's also ambiguous (is it a verb or a noun?). And the issue of whether
I think I came up with a solution, please see below.
it would just be a shorthand for sleep(inf) is still unsettled.
We can't fix sleep()
to support this. We can have N coroutines awaiting on sleep(inf)
and the user presses ^C: which coroutine should we dispatch the KeyboardInterrupt
to? Or maybe we have a server, and ^C happens in one of its socket read callbacks -- we can't dispatch that to sleep coroutines either.
Anyways, I think we can modify asyncio.run()
to accept asynchronous generators (in addition to regular coroutines). The closest analogy is @contextlib.contextmanager
decorator.
With that we can have this:
async def main():
server = await asyncio.start_server(handle_echo, '127.0.0.1', 8888)
try:
yield
finally:
server.close()
await server.wait_closed()
asyncio.run(main())
I think this is a great idea because:
forever()
or some other obscure coroutine.loop.run_forever()
or to add any new APIs to the loop.The yield
in this context means literally "yield control to the loop" from this "main" coroutine. If an exception happens or loop is stopped, please execute my cleanup code.
That's also ambiguous (is it a verb or a noun?)
I think it should rather be interrupted()
, as in Yury's server example above.
No need for forever() or some other obscure coroutine.
I like this, disregard my previous comment.
The latest patch is really great.
Using regular coroutine for client code and something like contextlib.contextmanager
for server ones fits pretty clean into my mind.
IIRC people are currently using a bare yield as the equivalent of sleep(0) -- just bounce to the loop, run all callbacks, and then continue. How can it also mean wait until the loop exits?
I really am very concerned that we're going to break things at the very last moment in a panic response to the critique from Nathaniel Smith.
Also frankly run_in_executor() is a pretty clumsy API due to the initial parameter that is usually None. Is it really important enough to have this? (Again, I worry that this is a panic response rather than something well thought out.)
IIRC people are currently using a bare yield as the equivalent of sleep(0) -- just bounce to the loop, run all callbacks, and then continue. How can it also mean wait until the loop exits?
But they use it in old style generator-based coroutines:
@coroutine
def foo():
yield # this is a NOP yield
async def main():
try:
yield # <- this is a yield from an async gen, something new in 3.6
Asynchronous generators can do any kind of yields they want -- there is no backwards compatibility issue here.
In fact, I'm thinking about making asyncio.run()
function Python 3.6 only, and clearly documenting both use cases.
I really am very concerned that we're going to break things at the very last moment in a panic response to the critique from Nathaniel Smith.
I wouldn't say that it's Nathaniel's post that caused this PR. I've been unhappy about the loop for a long time. I think I first proposed to fix get_event_loop
2 years ago.
Also frankly run_in_executor() is a pretty clumsy API due to the initial parameter that is usually None. Is it really important enough to have this? (Again, I worry that this is a panic response rather than something well thought out.)
Yes, I've been thinking about run_in_executor
too. I'll just drop it from this PR.
Oh, it's async generators. Too subtle.
We did fix get_event_loop(), and we're all happy with that. Can we just stop now please?
Again, if my voice cunts, the thing that I would like to have is a simple-minded run
(I could imagine this could be the same for other non-experts in asyncio
) an I will be glad if it will appear soon :-)
Only if it's obviously correct. The gyrations we're going through here suggest that it's not. (Or not yet.)
Again, if my voice counts, the thing that I would like to have is a simple-minded run (I could imagine this could be the same for other non-experts in asyncio) an I will be glad if it will appear soon :-)
If we don't handle the run_forever()
/cleanup, I think that the function will be useful only for 30% of use cases. I discovered how often we use run_forever
in asyncio docs and tutorials only yesterday. Then I looked at my own code, and realized that I myself can't use run()
anywhere unless we handle finalization.
@gvanrossum
Oh, it's async generators. Too subtle.
This is for the "main" function - will only be in the code once to bootstrap the program.
Almost no one knows about bare yield
expressions in generator-based coroutines. And a lot of asyncio users are just using async/await these days.
We did fix get_event_loop(), and we're all happy with that. Can we just stop now please?
Let me try to elaborate one last time on why I think it's essential to add this to 3.6:
shutdown_asyncgens
, etc.asyncio.forever()
was much more invasive. I'm confident that we won't have any problems with the latest design of run()
-- please look at the implementation.Speaking about Nathaniel's and Armin's posts -- they did highlight some real problems, and they resonated in Python community. I was giving a talk on PyCon Canada this weekend, and 50% of the questions there were about how complex asyncio is and how to learn it.
I code with asyncio every day, and meet with asyncio users very often. I'm trying hard to solve a real problem here, not just for beginners, but for advanced users like myself.
Then I looked at my own code, and realized that I myself can't use run() anywhere unless we handle finalization.
I am all in favour of careful set-up/clean-up.
I'm trying hard to solve a real problem here, not just for beginners, but for advanced users like myself.
It is quite interesting that advanced users and beginners ask about the same thing -- a simple way to "bootstrap" a program.
It looks like the only unsettled problem is what to do with run_forever
. I have no strong opinion on this now. But I don't think that we should decide right now. I will try to come up with some ideas tomorrow.
Then you are on your own. I have enough stress in my life without having to worry about this.
Some thoughts... (Guido, please ignore this if you don't have time or energy, you are already doing a lot for us :-))
Since there are two scenarios for the "top-level bootstrap", maybe there should be two functions run
and serve
. The semantics of run
is clear:
def run(coro):
# Set-up loop
result = loop.run_until_complete(coro)
# Careful clean-up
A possible semantics for serve
could be (approximately):
def serve(setup, teardown):
# Set-up loop
loop.run_until_complete(setup)
try:
loop.run_forever()
except:
...
result = loop.run_until_complete(teardown)
# Careful clean-up
There are however some problems with such semantics:
run_forever()
.The idea with an async generator that is now used by run
nicely solves both problems. So that a good possible option would be to have:
run(coro)
for simple use cases;serve(asyncgen)
for server uses cases (this one is 3.6+)Concerning the second one, I am also thinking maybe it should accept an arbitrary async context manager instead, and the possible implementation would be (note that I don't use async with
so that serve
will be a normal function, maybe this could be refactored via an inner coroutine):
def serve(mgr):
# Set-up loop
loop.run_until_complete(mgr.__aenter__())
try:
loop.run_forever()
except:
result = loop.run_until_complete(mgr.__aexit__(*sys.exc_info()))
else:
result = loop.run_until_complete(mgr.__aexit__(None, None, None))
# Careful clean-up
and then the logic that is now in run
could be transformed into @contextlib.asyncontextmanager
Some thoughts...
Thanks, Ivan! Some good research there. I'm going to write an email to async-sig and ask about this PR. There are few more people there that can provide feedback.
Concerning the second one, I am also thinking maybe it should accept an arbitrary async context manager instead
I'm not sure we need to push more features to asyncio.run
. So far I see two options:
asyncio.run()
that accepts generators or coroutines.asyncio.run()
that accepts coroutines; we add asyncio.run_forever()
that accepts async generators.If we manage to squeeze (1) or (2) in 3.6, we can add support for async context managers in 3.7.
Since my feedback was asked for via email: not sure how much it counts for anything but +1 on it conceptionally. However that being said I'm pretty sure the fact that this makes a new event loop and you not being able to run it on other threads is not super great. I will look at it in detail more but generally the fact that cleanup is so hard currently and inconsistent between libraries is absolutely not great.
I ran into the same issue multiple times already and it makes code super awkward in many ways, particularly in documentation and unittests.
[..] However that being said I'm pretty sure the fact that this makes a new event loop
Since the function closes the loop, I'm not sure it's a good idea to pass one inside. We have a mechanism to define a policy (you can do that in tests) with custom new_event_loop
function, so that you can inject your own preconfigured one (that's how uvloop works).
and you not being able to run it on other threads is not super great.
We can lift this requirement, it's not fundamental.
Well, this is an entire separate can of worms. I have huge problems with actually closing the loop in things like this if you have multiple things that need cleaning up. For instance I have some async generators that guard an aiohttp server and I cannot use the web.run
method because it closes the loop and does not provide a sensible way to make the async generator work with that.
I do not see how this function here would solve that issue either. Composability of async generators in the light of shutdowns and isolated loops is really tricky.
For a project of mine I ended up copy/pasting code from aiohttp because I could not make shutdowns work with the run method which closes the loop.
I do not see how this function here would solve that issue either. Composability of async generators in the light of shutdowns and isolated loops is really tricky.
Can you post an example where the proposed function won't help? The idea is that with asyncio.run()
you can do any kind of cleanup on your own in your main coroutine. The function then does some additional cleanup and closes the loop.
I don't think it's possible to implement a function that will help with all cases -- unittests is one area where you almost always need a lot of custom logic, and that's OK. In case of aiohttp
it's their job to provide you with good base unittest base classes and functions.
@1st1
I'm not sure we need to push more features to asyncio.run. So far I see two options:
I would really prefer option 2: two separate functions -- run
for coroutines and run_forever
for asyncgen's. I believe this could avoid confusion with old style coroutines mentioned by Guido.
I've modified this PR to add just two functions:
asyncio.run()
to run a coroutine:
async def main():
await asyncio.sleep(1)
print('hello')
asyncio.run(main())
asyncio.run_forever()
to run asyncio servers etc:
async def main():
server = await asyncio.start_server(...)
try:
yield # <- run the loop forever
finally:
server.close()
await server.wait_closed()
asyncio.run_forever(main())
This PR also adds unittests. FWIW I used a custom asyncio policy to control the loop that the new functions use during tests, and it worked great.
Thank you Yury, it looks great! I left few small comments concerning error messages.
Also I think maybe it is worth adding a note somewhere in docs for people who are already familiar with loop.run_until_complete
and loop.run_forever
that run
is a wrapper around loop.run_until_complete
that takes care of set-up and clean-up; and run_forever
is a wrapper around loop.run_forever
that adds the same set-up and clean-up logic around the logic provided by user in context-manager-like asyncgen?
(This is obvious if one looks at the code, but otherwise it might be not clear why such design was chosen.)
Also I think maybe it is worth adding a note somewhere in docs [..]
Sure, we'll update the docs!
We have very long discussion here.
As I see we have a preliminary agreement that .run()
is good addition.
After that @1st1 has remembered about server side code which usually uses run_forever
.
The first proposal for accepting a coroutine for client code and async context manager was working but not perfect solution.
Splitting it into run
and run_forever
make the API clean and obvious.
I'm +1 for the patch.
3.6b4 is scheduled for tomorrow, if there is a chance this goes into 3.6, then it probably makes sense to merge this before that time.
Can this PR be moved to python/cpython?
Can this PR be moved to python/cpython?
IIUC, Yury is working on a PEP now that will cover the features in this PR.
@ilevkivskyi on your example above on the exception I would actually name the special function differently.
instead of :
def serve(mgr):
# Set-up loop
loop.run_until_complete(mgr.__aenter__())
try:
loop.run_forever()
except:
result = loop.run_until_complete(mgr.__aexit__(*sys.exc_info()))
else:
result = loop.run_until_complete(mgr.__aexit__(None, None, None))
# Careful clean-up
I would do:
def serve(mgr):
# Set-up loop
loop.run_until_complete(mgr.__aenter__())
try:
loop.run_forever()
except:
result = loop.run_until_complete(mgr.__on_error__(*sys.exc_info()))
else:
result = loop.run_until_complete(mgr.__aexit__(None, None, None))
# Careful clean-up
So that way if they do not have or define an __aexit__
that can take in exception messages.
Or another way is to have some sort of decorator that would register an function with asyncio that would handle the exception message so that way asyncio would not error again if they do not have __on_error__
in the type or object.
This PR adds two new APIs:
asyncio.run()
andasyncio.run_in_executor()
. Ideally, if possible, I'd like to have them in 3.6. If I have a green light on the idea, I'll update the patch to add unittests.One of the main complaints that users have about asyncio is the situation around the event loop. We currently do a poor job explaining how exactly it should be used and how to structure asyncio programs in general.
With the recent update of
asyncio.get_event_loop()
we can now explain people that passing event loop explicitly is unnecessary, and that library APIs should be designed around coroutines.I think we need to add two more functions to make the loop disappear from most asyncio programs.
asyncio.run_in_executor()
coroutine: maps directly to the equivalentloop.run_in_executor()
. The idea is that people don't need an event loop to use the function:asyncio.run()
function: run a coroutine taking care of the asyncio event loop.Pros:
Simplification of the documentation: I'm working on an update, and one of the things that bothers me that to each and every example have to have a piece of code that manages the loop. For example:
The problem is that the above snippet isn't fully correct, we should have a
try-finally
block to ensure thatloop.close()
is always called. Even in the docs we don't do that. Withasyncio.run()
the snippet becomes much shorter:It's currently hard to experiment with asyncio in the REPL, because
get_event_loop
andrun_until_complete
are rather long names to type. Withasyncio.run()
:And
asyncio.run()
can be called multiple times.Asynchronous generators are properly cleaned-up.
loop.shutdown_asyncgens()
is a somewhat low-level advanced API, and I expect something that a lot of people will forget to use.The function promotes a coroutine-centric design. In many cases, is is possible to bootstrap an asyncio program with just one coroutine.
Cons:
It's not possible to completely substitute
loop.run_forever()
. One of the documented patterns in asyncio is to userun_forever
to bootstrap servers:To support cases like this, we'll need to add another API. One of the ideas that I have (not for 3.6!) is to add
asyncio.forever()
awaitable, so that the above example could be translated to:Adding
asyncio.forever()
would require us to add new APIs to event loop, and it is something that clearly requires a thorough review process (I'm thinking about writing a PEP).However, we can probably add
asyncio.run()
function in 3.6 to cover some use cases, and enhance it further in 3.7.@gvanrossum, what do you think?