[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Andrew Svetlov]

Andrew Svetlov  added the comment:

Implemented by #32591

--
resolution:  -> duplicate
stage: patch review -> resolved
status: open -> closed
superseder:  -> Deprecate sys.set_coroutine_wrapper and replace it with more 
focused API(s)

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Guido van Rossum]

Guido van Rossum  added the comment:

Serhiy, that’s brilliant! Let’s do it.

--
nosy: +Guido.van.Rossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

CALL_FUNCTION ...
WARN_IF_AWAITABLE_AND_POP_TOP

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nicholas Smith]

Change by Nicholas Smith :


--
nosy: +nzsmith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nikolay Bryskin]

Change by Nikolay Bryskin :


--
nosy: +nikicat

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Andrew Svetlov]

Andrew Svetlov  added the comment:

Let's keep it open if you don't give up the issue entirely.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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...?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Andrew Svetlov]

Andrew Svetlov  added the comment:

This is a new feature (and not a blocker).
Shift to 3.9.

Nathaniel, the PR is outdated.
Have you an intention to land it or the issue can be closed by the lack of 
interest?

--
nosy: +asvetlov
versions: +Python 3.9 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Ryan Hiebert]

Change by Ryan Hiebert :


--
nosy: +ryanhiebert

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Cheryl Sabella]

Cheryl Sabella  added the comment:

Hi Nathaniel,

Was this something you were still targeting for 3.8?

Thanks!

--
nosy: +cheryl.sabella

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Xavier G. Domingo]

Change by Xavier G. Domingo :


--
nosy: +xgdomingo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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] https://github.com/python-trio/trio/issues/79
[2] https://bugs.python.org/issue30491#msg294584
https://github.com/python-trio/trio/issues/79#issuecomment-304428955
[3] https://github.com/python-trio/trio/pull/176
[4] https://github.com/python-trio/trio/issues/79#issuecomment-325188030
[5] https://github.com/pytest-dev/pytest-asyncio/issues/67
[6] https://bugs.python.org/issue30491#msg308090
[7] https://bugs.python.org/issue30491#msg310559

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

So let's retarget this to 3.8 as it's too close to 3.7 feature freeze to get 
seriously considered/merged.

--
versions: +Python 3.8 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Guido van Rossum]

Guido van Rossum  added the comment:

No. I just mailed Yury with an explanation.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

Thanks for the summary, Yury! One quick note:

> Effectively, the previously merged origin-tracking API (the one with which we 
> replaced set_coroutine_wrapper) achieves the same goal.

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Change by Yury Selivanov :


--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

Guido,

This is another feature for native coroutines that Nathaniel proposes for 3.7.

Here's a summary (this issue has too many messages):

1. It adds a new mechanism to detect un-awaited coroutines (i.e. when a user 
forgets to use await).

2. To enable the mechanism, which is disabled by default, a "sys. 
set_unawaited_coroutine_tracking_enabled()" function needs to be called by a 
framework.

3. "sys.get_unawaited_coroutines()" returns a list of not-yet-awaited 
native-coroutine objects that were created since the previous call to 
"get_unawaited_coroutines()".

4. In Trio, the APIs are designed to accept *coroutine functions*.  So in Trio, 
a user either writes "await coro()" or passes "coro" to a Trio API.  Nathaniel 
wants Trio to check periodically if there are any un-awaited coroutines and 
raise an error, which should improve Trio usability.

5. Another potential user of this new functionality is pytest-asyncio. It can 
use the API to warn the user that a test created some coroutines that were 
never awaited. 

6. asyncio/tornado/curio/twisted will not be able to use this API.

7. Effectively, the previously merged origin-tracking API (the one with which 
we replaced set_coroutine_wrapper) achieves the same goal.  The only downside 
is that the warning won't be raised until GC.  On other interpreters like PyPy 
it can take a while before a coroutine gets GCed.

8. The patch adds two pointers to native coroutines objects to maintain a 
doubly-linked list of them.

9. The performance impact is visible in micro-benchmarks, but is unlikely be 
visible in real applications.  Still, it's about 3-4% slowdown for both 
generators and coroutines.

10. The PR itself is in a good shape, although I'll still make a review pass if 
you approve this feature to go in 3.7.

What do you think?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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 
binary(19) * 30: total 7.349s
abinary(19) * 30: total 7.727s
~/src/cpython$ with-unawaited-tracking/install/bin/python3 pep492bench.py 
binary(19) * 30: total 7.758s
abinary(19) * 30: total 8.023s
~/src/cpython$ without-unawaited-tracking/install/bin/python3 pep492bench.py
binary(19) * 30: total 7.326s
abinary(19) * 30: total 7.686s
~/src/cpython$ with-unawaited-tracking/install/bin/python3 pep492bench.py   
binary(19) * 30: total 7.652s
abinary(19) * 30: total 7.999s
~/src/cpython$ without-unawaited-tracking/install/bin/python3 pep492bench.py
binary(19) * 30: total 7.421s
abinary(19) * 30: total 7.732s
~/src/cpython$ with-unawaited-tracking/install/bin/python3 pep492bench.py   
binary(19) * 30: total 7.541s
abinary(19) * 30: total 8.132s

-

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)
Out[1]: 1.0386947863866764

~3.9% slower.

And for coroutines, with-unawaited-tracking is:

In [2]: (8.023 + 7.999 + 8.132) / (7.727 + 7.686 + 7.732)
Out[2]: 1.043594728883128

~4.4% slower.

--
Added file: https://bugs.python.org/file47406/pep492bench.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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 3954f6126f (head of my PR branch, labeled "with-unawaited-coroutines" 
below) and f23746a934 (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
.
coroutine creation/close: Mean +- std dev: 3.90 us +- 0.11 us
~/src/cpython$ with-unawaited-tracking/install/bin/python3 corobench.py   
.
coroutine creation/close: Mean +- std dev: 3.90 us +- 0.09 us
~/src/cpython$ without-unawaited-tracking/install/bin/python3 corobench.py
.
coroutine creation/close: Mean +- std dev: 3.93 us +- 0.21 us
~/src/cpython$ with-unawaited-tracking/install/bin/python3 corobench.py   
.
coroutine creation/close: Mean +- std dev: 3.91 us +- 0.10 us


Script: attached as "corobench.py"

--
Added file: https://bugs.python.org/file47405/corobench.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

I'd like to see benchmarks of coroutine creation time and awaits with and 
without the proposed patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Change by Nathaniel Smith :


--
keywords: +patch
pull_requests: +5123
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

I have a patch for this, but it will be simplest if we can merge 
https://github.com/python/cpython/pull/5250 (for bpo-32591) first, because they 
touch a lot of overlapping code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

> In any case, in my opinion, it doesn't matter.  `sys.set_coroutine_wrapper` 
> controls a single thread-local setting, 
> `sys.set_unawaited_coroutine_tracking` *also* controls a single thread-local 
> setting.  Both of them have the exact same problem when there's more than one 
> user of the API.  So we can't consider this as a strong argument in favour of 
> the 'set_unawaited_coroutine_tracking' API.
[...]
> 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)?

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 o
 f 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

> So first... that's not how nursery.start_soon works :-). It actually takes an 
> async function, not a coroutine object. [..]

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 coro wrapper in Trio.  Maybe the runtime overhead of it will be 
negligible.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the comment:

> 1. How will trio handle situations like:
> 
> c = coro()
> await ...
> nursery.start_soon(c)
> 
> ?
[...]
> 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.

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):

> TypeError: trio was expecting an async function, but instead it got a 
> coroutine object 
> 
> Probably you did something like:
> 
>   trio.run(coro(...))# incorrect!
>   nursery.start_soon(coro(...))  # incorrect!
> 
> Instead, you want (notice the parentheses!):
> 
>   trio.run(coro, ...)# correct!
>   nursery.start_soon(coro, ...)  # correct!

(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?".

Buuut... 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 :-).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

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).

>> 2. What if another framework (say asyncio that you want to interoperate 
>> with) calls "sys.set_unawaited_coroutine_tracking(False)"?  

>> The API you propose has the *same* pitfall that 
>> 'sys.set_coroutine_wrapper()' has, when used by several frameworks 
>> simultaneously.

> As far as I understand you'll need to yield back to the other framework at 
> some
> point. Then it's IMHO to the author to make sure to save the state of
> `sys.collect_unawaited_coroutines()`, set
> `sys.set_unawaited_coroutine_tracking(enabled: bool)` to what the other
> framework expect, and  do the opposite when they get control back. 

Who do you refer to, when you say "author"?

In any case, in my opinion, it doesn't matter.  `sys.set_coroutine_wrapper` 
controls a single thread-local setting, `sys.set_unawaited_coroutine_tracking` 
*also* controls a single thread-local setting.  Both of them have the exact 
same problem when there's more than one user of the API.  So we can't consider 
this as a strong argument in favour of the 'set_unawaited_coroutine_tracking' 
API.

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Matthias Bussonnier]

Matthias Bussonnier  added the comment:

Let me try to explain better, I'm pretty sure there is just a misunderstanding
from some of use in the vocabulary or presupposition we start from. I have the
impression that you feel like the API will automatically make coroutine raise
when they are not awaited, while I have the impression that in only provide a
tool for frameworks to detect unawaited coros and raise As soon as possible in
the right place.


We have the scheduler/eventloop that advance users tasks. Each time a user task
yield to the scheduler directly, or indirectly, the scheduler want to check that
since last time it was called in the same task no non-awaited coroutines are
pending, that is to say

await sleep(0) # yield to scheduler
c1 = coro() 
await c1

c2 = coro()

c3 = coro()
del c3
gc.collect()

await sleep(0) # yield again.


We want to make sure that between the two `sleep(0)` no coro is unawaited. Let's
assume `await coro()` does not itself yield to the scheduler for simplicity. In
above case you can see that c2 is not awaited, so according to Nathaniel
proposal, `sys.collect_unawaited_coroutines()` would returns `[c2, c3]`, So the
second sleep(0) can raise NonAwaitedCoroutineError, (and if tracemalloc is
enabled give more informations). 



You propose to use 
  weakref.ref(coro, trio._untrack_coro)

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:

> the weak reference object will be passed as the only parameter to the
> callback; *the referent will no longer be available*.

Thus we cannot check the state of the collected coroutine, according to my
understanding and experiments (Also we need to keep ref alive, but that's a
minor inconvenience). My guess is the referent is not around for users not to
suddenly add a ref to it...

So such a tracked list could only be use to know if coroutines where GC'd or
not.


If you are using wekrefs, you may also think about
`sys.collect_unawaited_coroutines()` returning weakref list, but that would lead
to `WeakRefList[c2]` because c3 was collected :-(. While we still raise because 
of
c2, a user logic only containing c3 would pass the "no unawaited coroutines".

To respond to your answer more specifically:

> 1. How will trio handle situations like:

>c = coro()
>await ...
>nursery.start_soon(c)


It depends what your `...` is, if its yielding to the trio scheduler, then the
scheduler will check unawaited coroutines, see that the list is non-empty and
raise a NonAwaitedCoroutineError at your `await ...`. In the case your `await
...` does not yield.. then nothing. 

> 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.

Yes, and the trio/curio/asyncio library is responsible to check
`sys.collect_unawaited_coroutines()`, and react appropriately. Trio will likely
decide to raise. Asyncio can just forget this option exists.

> 2. What if another framework (say asyncio that you want to interoperate with) 
> calls "sys.set_unawaited_coroutine_tracking(False)"?  

> The API you propose has the *same* pitfall that 'sys.set_coroutine_wrapper()' 
> has, when used by several frameworks simultaneously.

As far as I understand you'll need to yield back to the other framework at some
point. Then it's IMHO to the author to make sure to save the state of
`sys.collect_unawaited_coroutines()`, set
`sys.set_unawaited_coroutine_tracking(enabled: bool)` to what the other
framework expect, and  do the opposite when they get control back. 


> 3. Given (2), why don't you have a coroutine wrapper like this:

>  def trio_coroutine_wrapper(coro):
>  ref = weakref.ref(coro, trio._untrack_coro)
>  trio._track_coro(ref)
>  return coro

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
do here[1], except now everytime you need to check
`inspect.getcoroutinestate(coro) == 'CORO_CREATED'` on every item. You can't
use weakref callback as you can't check the coro state in this case. The check
on all coro everytime does add potentially significant overhead, AND block the
GC of awaited couroutines in between checks. While we _do want_ to  block GC of
coroutine only if `sys.collect_unawaited_coroutines()` is set to true and
coroutine is not awaited.

Also we like to keep the actual non-awaited coro around for error messages, but
I guess we can do without. 

One alternative would be the ability to add a callback when coros change
states... But I though coroutines where one way to avoid callbacks...


Now you have way more understanding of core CPython than me, and I may 

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

>   send_ping() # don't care about result, forgot await
>   # get collected
>   await something_that_will_trigger_check_coros_weakreaf() # oh no !

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.

> So if its weak-refd then you can't be sure you are actually preventing 
> non-awaited coros.

Sorry, I don't understand this sentence either.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Matthias Bussonnier]

Matthias Bussonnier  added the comment:

Your last description is about exactly what 
https://github.com/python-trio/trio/pull/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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Yury Selivanov]

Yury Selivanov  added the comment:

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:

1. How will trio handle situations like:

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.


2. What if another framework (say asyncio that you want to interoperate with) 
calls "sys.set_unawaited_coroutine_tracking(False)"?  

The API you propose has the *same* pitfall that 'sys.set_coroutine_wrapper()' 
has, when used by several frameworks simultaneously.


3. Given (2), why don't you have a coroutine wrapper like this:

   def trio_coroutine_wrapper(coro):
   ref = weakref.ref(coro, trio._untrack_coro)
   trio._track_coro(ref)
   return coro

This way:

- you don't introduce a lot of overhead, because there's no actual wrapper 
around a coroutine

- you can capture the file/lineno at the point where a coroutine was created, 
which is useful for reporting unawaited coroutines

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

Nathaniel Smith  added the 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 c
 all 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:

- The original comment above suggested keeping a count of unawaited coroutines 
instead of tracking the actual objects, but this way is just about as cheap 
while (a) allowing for much better debugging information when an unawaited 
coroutine is detected, since you have the actual objects there and (b) avoiding 
a mess of issues around unawaited coroutines that get GCed before the user 
checks for them.

- What about using the existing coroutine wrapper hook? You could do this, but 
this proposal has two advantages. First, it's much faster, which is important 
because Trio wants to run with this enabled by default, and pytest-asyncio 
doesn't want to slow down everyone's test suites too much. (I should benchmark 
this properly, but in general the coroutine wrappers add a ton of overhead b/c 
they're effectively a whole new Python-level object being allocated on every 
function call.) And second, since the coroutine wrapper hook is such a generic 
mechanism, it's prone to collisions between different uses. For example, 
pytest-asyncio's unawaited coroutine detection and asyncio's debug mode seem 
like they ought to complement each other: pytest-asyncio finds the problematic 
coroutines, and then asyncio's debug mode gives the details on where they came 
from. But if they're both trying to use the same coroutine wrapper hook, then 
they'll end up fighting over it. So this proposal follows Python's
  general rule that generic hooks are fine when you really need an escape 
hatch, but if there's a specific use case it's often worth handling it 
specifically. (Recent example: module __class__ assignment vs. PEP 562.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Chris Jerdonek]

Changes by Chris Jerdonek :


--
nosy: +chris.jerdonek

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Matthias Bussonnier]

Changes by Matthias Bussonnier :


--
nosy: +mbussonn

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30491] Add a lightweight mechanism for detecting un-awaited coroutine objects

[Nathaniel Smith]

New submission from Nathaniel Smith:

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: 
https://github.com/python-trio/trio/issues/79#issuecomment-304364010)

--
components: asyncio
messages: 294584
nosy: giampaolo.rodola, haypo, ncoghlan, njs, yselivanov
priority: normal
severity: normal
status: open
title: Add a lightweight mechanism for detecting un-awaited coroutine objects
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com