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
Add a lightweight mechanism for detecting un-awaited coroutine objects #74676
Comments
A common problem when working with async functions is to attempt to call them but forget the 'await', which eventually leads to a 'Warning: coroutine ... was never awaited' (possibly buried in the middle of a bunch of traceback shrapnel caused by follow-on errors). This can be confusing, lead to spuriously passing tests, and generally isn't a great experience for the dev who makes the mistake. To improve this, I'd like to do things like have a test harness reliably detect when this has happened inside a test case, or have trio's main loop check occasionally to see if this has happened and immediately raise a useful error. (Unfortunately, it *doesn't* work to promote the warning to an error using the usual warnings filter machinery, because the warning is raised inside a __del__ method, which suppresses exception propagation.) In principle this is possible with sys.setcoroutinewrapper, but that adds non-trivial runtime overhead to every async function invocation, which is something we'd like to avoid. (It's OK for a "debug mode", but "modes" are also a poor dev experience: they certainly have their place, but whenever possible it's much nicer to give a proper error in the first place instead of relying on the dev to realize they need to enable debug mode and then remember how to do it.) Therefore I propose that CPython keep a thread-local counter of how many coroutine objects currently exist in a created-but-not-yet-started state -- so corofn.__call__ would increment this counter, and the first call to coroobj.__next__/send/throw would decrement it. And there's some way to access it e.g. via a magic function in the sys module. Then test harnesses can assert that this is zero, trio could occasionally poll this from its run loop and assert that it's zero, etc., with very low overhead. (This is a slight modification of the version I discussed with Yury and others at PyCon last week; the previous request was to keep a count of how many times the "coroutine '...' was never awaited" warning had been emitted. The problem with the original idea is that the warning message doesn't fire until the garbage collector has collected the coroutine object, and that might not happen at a convenient time if we're using PyPy, or if there are cycles, or in a test where the missing 'await' eventually leads to an exception whose traceback pins the coroutine object in memory just long enough for the warning to be detected on the *next* test and confuse everyone. Thanks to Matthias Bussonnier for spurring the new idea, and see discussion here: python-trio/trio#79 (comment)) |
Update! I've been experimenting with this some more, and here's a more detailed proposal, that I'd ideally like to get into 3.7. I don't *think* this is big enough to need a PEP? I dunno, thoughts on that welcome. Motivation: It's easy to accidentally write 'f()' where you meant 'await f()', which is why Python issues a warning whenever an unawaited coroutine is GCed. This helps, and for asyncio proper, it may not be possible to do better than this -- since the problem is detected at GC time, there's very little we can do *except* print a warning. In particular, we can't raise an error. But this warning is still easy to miss, and prone to obscure problems: it's easy to have a test that passes ... because it didn't actually run any code. And then the warning is attached to a different test entirely. But, in some specific cases, we could do better: for example, if pytest-asyncio could check for unawaited coroutines after each test, it could immediately raise a proper and detailed error on the correct test. And if trio could check for unawaited coroutines at selected points like schedule points, it could reliably detect these problems and raise them as errors, right at the source. Specification: We add two new functions, sys.set_unawaited_coroutine_tracking(enabled: bool) -> None and sys.collect_unawaited_coroutines() -> List[Coroutine]. The semantics are: internally, there is a thread-local bool I'll call tracking_enabled that defaults to False. set_unawaited_coroutine_tracking lets you set it. If tracking_enabled == False, everything works like now. If tracking_enabled == True, then the interpreter internally keeps a table of all unawaited coroutine objects: when a coroutine object is created, it's automatically added to the table; when it's awaited, it's automatically removed. When collect_unawaited_coroutines is called, it returns the current contents of the table as a list, and clears it. The table holds a strong reference to the coroutines in it, which makes this is a simple and reliable way to track unawaited coroutines (but also means that we need the enable/disable API instead of leaving it on all the time, because once it's enabled someone needs to call collect_unawaited_coroutines regularly to avoid a memory leak). Implementation: this can be made fast and cheap by storing the table as a thread-specific intrusive double-linked list. Basically each coroutine object would gain two pointer slots (this adds a small amount of memory overhead, but a coroutine object + frame is already >500 bytes, so the relative overhead is low), which are used to link it into a list when it's created (O(1), very cheap), and then unlink it again when it's awaited (also O(1), very cheap). Rejected alternatives:
|
First, I've no questions about the proposed implementation. It shouldn't have performance impact when unawaited coroutine tracking is off, which is the default. It will cause minimal overhead when the tracking is on, which is fine. Adding two extra pointers to coroutine objects should be OK too. Back to the specification. Few questions:
c = coro()
await ...
nursery.start_soon(c) ? There's a brief period of time when "c" is not being awaited, and thus it will be in the list that "sys.collect_unawaited_coroutines()" returns. How exactly do you plan to use this new API in Trio? Maybe creating a coroutine and not immediately passing it to 'start_soon' or similar API is an anti-pattern in Trio, but it is an OK thing to do in asyncio/curio/twisted.
The API you propose has the *same* pitfall that 'sys.set_coroutine_wrapper()' has, when used by several frameworks simultaneously.
def trio_coroutine_wrapper(coro):
ref = weakref.ref(coro, trio._untrack_coro)
trio._track_coro(ref)
return coro This way:
|
Your last description is about exactly what python-trio/trio#176 is about (which I need to resurect) There are some issue with weakref some that I don't remember, but one of them is (IIRC): what if a non-awaited coro get collected before being awaited and check by trio ? send_ping() # don't care about result, forgot await
# get collected
await something_that_will_trigger_check_coros_weakreaf() # oh no ! So if its weak-refd then you can't be sure you are actually preventing non-awaited coros. By default in trio we also would like to enforce immediate coro awaiting, but we of course would provide a convenience function to wrap coroutines you really do not want to await now. (Explicit is better than implicit) So you would write: c = coro()
really_not_awaiting_now_leave_me_alone(c)
await ... #
nursery.start_soon(c) What above PR does is check each time you enter the trio scheduler whether there is unawaited Coros (not marked as really_no_not_awaiting_now), and if so, next time you restart this task (or enter an cancellation point.. but details) raise a NonAwaitedCoroutineError. It does not raise _exactly_ where it was not awaited but narrow down where the non-awaited coro is (between two schedule point). And _if_ you enable tracemalloc the error message give you where the stack trace that created this coro. |
I don't understand what you are trying to say with this example. What is "something_that_will_trigger_check_coros_weakreaf" supposed to do? The point of my proposal that you will have a coroutine reported at the "get collected" point in CPython, or sometime later in PyPy.
Sorry, I don't understand this sentence either. |
Let me try to explain better, I'm pretty sure there is just a misunderstanding We have the scheduler/eventloop that advance users tasks. Each time a user task
c2 = coro()
c3 = coro()
del c3
gc.collect()
We want to make sure that between the two You propose to use And I suppose you want us to do def _untrack_coro(self, ref):
coro = ref()
if inspect.getcoroutinestate(c) == 'CORO_STARTED'
.. do what's necessary for unawaited coroutine.
else:
# just discard from the tracked list. Unfortunately:
Thus we cannot check the state of the collected coroutine, according to my So such a tracked list could only be use to know if coroutines where GC'd or If you are using wekrefs, you may also think about To respond to your answer more specifically:
It depends what your
Yes, and the trio/curio/asyncio library is responsible to check
As far as I understand you'll need to yield back to the other framework at some
We can't use weakref (cf issue with c3/weakref above). We could track all coroutines, with a normal container, which is exactly what we Also we like to keep the actual non-awaited coro around for error messages, but One alternative would be the ability to add a callback when coros change Now you have way more understanding of core CPython than me, and I may just not 1: https://github.com/python-trio/trio/pull/176/files#diff-ce66495f4b73d01ba38f06ae87955d3aR54 |
Matthias, Thanks a lot for such a detailed write-up! I now better understand how you want to use the proposed API in Trio, and why the weakref approach isn't going to work (at least not in a straightforward way).
Who do you refer to, when you say "author"? In any case, in my opinion, it doesn't matter. We can fix asyncio to manage coroutine wrappers more responsively. Right now it will raise an error if there's a wrapper set by another framework. Ideally, event loops should get the current wrapper, set their own, and then restore the original wrapper when they are done. With that all in mind, my question is: have you guys tried to write a wrapper object in C for coroutines (or you can use Cython to get an idea)? You can have a freelist of such objects to make their instantiation super cheap. And you only need to proxy the '__await__' method (tp_as_async.am_await). |
Yeah, for sure. This is sort of core to the overall motivation, so probably a good idea to make sure we're on the same page. So first... that's not how nursery.start_soon works :-). It actually takes an async function, not a coroutine object. In fact, if you try doing 'nursery.start_soon(c)' in trio v0.2.0, you get this error message (code I ran: https://gist.github.com/njsmith/87687ba5de1aeb5aa92336bd31891751):
(The 'coro' in my paste above isn't a placeholder, it's the actual name of your example async function that trio has interpolated into the error message, because I am a dorky perfectionist.) There's a broader point here beyond showing off my dorkiness. Trio is very careful to make sure that users don't need to know about the existence of coroutine objects *at all*. This is a *huge* win for teaching and understanding: I just tell people "async functions are a special class of functions; they're defined like 'async def func(): ...', and called like 'await func()', and btw you can only use 'await func()' inside an async function." and that is literally everything you need to know about async/await to use trio. I never use the word coroutine. It's way simpler, with no loss in expressivity, and part of why I can teach all of trio in a 30 minute talk and people are like "okay cool I know how to write concurrent programs now, why do people make such a big deal about it?". Buuuuuuut... that's the happy path. What happens when people make minor syntax mistakes? Sometimes, we can catch it -- like the example above, when they accidentally write 'nursery.start_soon(func())' -- and give a nice helpful message to tell them what to fix, and it's fine. But unawaited coroutines are a problem: most people are going to forget an 'await' sooner or later. And when the happens, then you get messages like "coroutine was never awaited" or "coroutine object has no attribute '...'" which are completely incomprehensible if you don't know that there are such things as coroutine objects. (And never mind the cases where you don't get an error/warning at all!) So then my users are forced to learn a bunch of new concepts and async/await implementation details, *just* to understand this one error case. So basically yeah, I am totally happy to prohibit 'c = coro(); await sleep(0); await c' because it's actually the key thing that makes the whole system simpler. Of course the problem is how to do this in a way that works for both trio and asyncio :-). |
Interesting, and I think I like it. I definitely understand the motivation to not tell users the difference between coroutine functions and coroutine objects, but yeah, the ship has sailed for asyncio/curio/twisted. Anyways, I'm not saying we shouldn't implement your proposal, even if it is only going to be used by Trio. I'm saying that we need to consider all alternatives, and for that, I'd like to see what happens if we implement a tiny&fast coro wrapper in Trio. Maybe the runtime overhead of it will be negligible. |
This is a fair point; we probably should try that :-). I do think that there's more of a difference than you're realizing between set_coroutine_wrapper and set_unawaited_coroutine_tracking WRT their behavior around conflicts. For set_unawaited_coroutine_tracking, the whole idea that makes it useful is if you have a single central piece of code that has a global view of what your program is doing, and can use that to figure out specific points where there should be no unawaited coroutines, plus has some strategy for reporting errors. For pytest-asyncio, it wants to check for these at the end of each test and then if found, fail that test. (Oh, I should mention that I did actually talk to the pytest-asyncio author about this and they do want to use it, this isn't hypothetical :-).) For trio, we have a more subtle rule like "check at each schedule point or catch-all exception handler, UNLESS it is in a task that is hosting asyncio code through our translation layer", and the reporting is more complicated too. But the point I want to make is that here, it's simpliy intrinsic in the design that whatever system is doing this has to have some pretty solid global knowledge, and it doesn't make sense to have two of these at once; no matter how we implemented this, two libraries trying to use it at the same time would necessarily need to coordinate somehow. OTOH, for set_coroutine_wrapper, it's such a catch-all that it is plausible to have two different libraries that both want to use it at the same time, for two things that in principle don't conflict at all. And not only is this plausible, it's what actually happens :-). If everyone uses set_coroutine_wrapper, then pytest-asyncio and asyncio will actually collide, and trio and asyncio will actually collide. But if we have set_unawaited_coroutine_tracking + set_coroutine_wrapper, then that's enough to prevent all the actual collisions we know about. |
I'd like to see benchmarks of coroutine creation time and awaits with and without the proposed patch. |
Please insert the usual caveats around how reliable benchmarking is impossible. (Last month when I tried this with a previous version of the patch, the interpreter that had the patch applied -- and thus was doing slightly *more* work -- was consistently a few percent faster in general, because something something PGO I guess? Compilers are weird.) That said, I'm not seeing any measurable difference at all with this patch. This isn't surprising: in the default mode (tracking disabled), the extra operations are: coroutine creation: 'if (predictably_true) { self->a = NULL; self->b = NULL }' send/throw: 'if (genobject->ob_type == PyCoro_Type && self->a != NULL) { /* not executed, because the condition always fails */ }' ----- benchmark details I built 3954f61 (head of my PR branch, labeled "with-unawaited-coroutines" below) and f23746a (its immediate parent, on master, labeled "without-unawaited-coroutines" below), with --enable-optimizations, on a Thinkpad T450s running 64-bit Linux. I ran a script (included below) that simply created and executed a trivial coroutine over and over, and measured using perf. 2 runs of each, alternating: ~/src/cpython$ without-unawaited-tracking/install/bin/python3 corobench.py Script: attached as "corobench.py" |
Yury also asked me to try running a generator/coroutine microbenchmark from PEP-492 (https://www.python.org/dev/peps/pep-0492/#async-await). I'm attaching the actual script for that as well (pep492bench.py), since I had to add a few lines to actually run the functions in the PEP :-). Results from 3 runs each of the two builds, alternating: ----- ~/src/cpython$ without-unawaited-tracking/install/bin/python3 pep492bench.py ----- So here we get a small difference between the with-unawaited-tracking and the without-unawaited-tracking builds. For generators, with-unawaited-tracking is: In [1]: (7.541 + 7.652 + 7.758) / (7.349 + 7.326 + 7.421) ~3.9% slower. And for coroutines, with-unawaited-tracking is: In [2]: (8.023 + 7.999 + 8.132) / (7.727 + 7.686 + 7.732) ~4.4% slower. |
Guido, This is another feature for native coroutines that Nathaniel proposes for 3.7. Here's a summary (this issue has too many messages):
What do you think? |
Thanks for the summary, Yury! One quick note:
I would say the two features are complementary. This feature (unawaited tracking) is cheap (a few % slowdown on microbenchmarks), and lets you find all the unawaited coroutine objects created in a given span of code. Origin-tracking is expensive (~3x slowdown on microbenchmarks with asyncio's default settings), and lets you take a coroutine object and map it back to the exact line of code where it was created. If you have both, then you can find all the coroutine objects and then find their exact origins. In the testing use case, there are two problems with relying on the GC to issue warnings: (1) because of traceback frame capture, reference cycles, etc., currently "coroutine was never awaited" warnings often (usually?) get attributed to the wrong test. (And yeah, it's even worse on pypy.) (2) because it's just a warning, it's easy to miss entirely -- if you forget an 'await' you can easily get a test that passes because it never actually executed any code (!), and when the tests are green most people don't read the CI logs to check for warnings. The idea of this feature is it's cheap enough for pytest-asyncio or trio to turn it on by default, and it makes the detection deterministic, so e.g. pytest-asyncio can reliably figure out which test caused the problem and mark it as FAILed. Then if it you also turn on origin-tracking, it can further narrow that down to the exact line in the test that caused the failure. |
No. I just mailed Yury with an explanation. |
So let's retarget this to 3.8 as it's too close to 3.7 feature freeze to get seriously considered/merged. |
I *think* I have a better idea than this for 3.8, but it's definitely PEP-sized. The unawaited coroutine tracking proposal here was more conceived as a small, unobtrusive thing that could be added to 3.7 and marked provisional, to hold us over until 3.8. (Just this week I've spent time helping two people who were having trouble with forgetting 'await's, and this feature would have given them better errors -- though one was also affected by bpo-32703.) OTOH the issue is complicated enough that it's entirely possibly my "better idea" has missed some showstopper detail, in which case maybe this is the best solution after all. We'll have plenty to talk about at PyCon this year :-). Also, looking at the issue log here there's a ton of missing context from all the discussions that happened elsewhere this year and eventually led to this particular proposal; I'll try to make a list of links and post it here for future reference and archaeologists. |
Okay, more context for future archaelogists / me in 6 months, phrased in the form of a timeline: This discussion started back in March, when I made an issue on the trio tracker [1] with notes on rough edges in CPython that it might make sense to try to target for 3.7; Brett, Yury, Nick, Dave Beazley, Matthias Bussonier joined in and it quickly turned into a discussion around the question of "is there anything we can do to improve developer experience when they forget an await?". Various impractical ideas were brainstormed / critiqued / prototyped. From discussions at PyCon in May, I realized that while catching missing 'await' where they happen is very difficult, an unobtrusive tweak to the interpreter might allow catching them soon after they happen; that's when I filed this issue (bpo-30491) with a vague initial proposal [2]. Matthias also did some work on prototyping this using set_coroutine_wrapper [3] End of August/beginning of September: more detailed analysis of various tricky edge cases refined this into an design that could actually work [4]; checked with pytest-asyncio devs for their thoughts [5]. Early/Mid December 2017: uh oh, feature freeze is less than 2 months away, better get moving; I posted a full proposal here [6]. After a few rounds of hashing out the basic idea here in the issue tracker, we seemed to have consensus that this was a reasonable approach, but (a) would it be better to add the feature to the interpreter, or to put it in a third-party library using set_coroutine_wrapper? and (b) how do we get this and asyncio debug mode to work together, like you'd want, instead of clashing over the coroutine wrapper and generally making a mess? Mid January 2018: Yury re-emerges from vacation, and in more discussions we realized that implementing "origin tracking" and "unawaited tracking" as built-in features would not only cover all the use cases, but would also open up origin tracking to other async frameworks, allow the two tools to complement each other instead of colliding, and let us deprecate set_coroutine_wrapper (which is a pretty obviously terrible API, much uglier than the two more specialized features here). So I filed bpo-32591 and wrote a patch for it, which was merged, and then reworked my prototype for this (bpo-30491) into something reviewable (gh-5279). Tuesday: Yury realized "oh $#@ in all of that we never talked to Guido" [7]. Lesson learned! [1] python-trio/trio#79 |
Hi Nathaniel, Was this something you were still targeting for 3.8? Thanks! |
This is a new feature (and not a blocker). Nathaniel, the PR is outdated. |
It's something I'm still interested in, but I'm not actively working on it (as you noticed :-)), and there are some other CPython changes that I'll probably prioritize first. Do you want to close this and I can re-open it when I do get back to it, or...? |
Let's keep it open if you don't give up the issue entirely. |
The most common error is missing keyword "await" in function call. "f()" instead of "await f()". There is a way to detect this error at runtime with minimal false positive and with minimal overhead. We can add a new opcode which checks if the value on the top of the stack is awaitable and raise warning/error in that case, and add it after every functional call whose result is ignored in asynchronous functions. It could even be merged with POP_TOP to reduce overhead.
|
Serhiy, that’s brilliant! Let’s do it. |
Implemented by bpo-32591 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: