
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2018-01-29 04:06 bynjs, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 5410 | merged | yselivanov,2018-01-29 06:15 | |
| Messages (13) | |||
|---|---|---|---|
| msg311052 -(view) | Author: Nathaniel Smith (njs)*![]() | Date: 2018-01-29 04:06 | |
Example (minimal version ofhttps://github.com/python-trio/trio/issues/425):-----async def open_file(): passasync def main(): async with open_file(): # Should be 'async with await open_file()' passcoro = main()coro.send(None)-----Here we accidentally left out an 'await' on the call to 'open_file', so the 'async with' tries to look up 'CoroutineType.__aexit__', which obviously doesn't exist, and the program crashes with an AttributeError("__aexit__"). Yet weirdly, this doesn't trigger a warning about 'open_file' being unawaited. It should!Yury's theory: maybe BEFORE_ASYNC_WITH's error-handling path is forgetting to DECREF the object. | |||
| msg311053 -(view) | Author: Nathaniel Smith (njs)*![]() | Date: 2018-01-29 04:10 | |
> Yury's theory: maybe BEFORE_ASYNC_WITH's error-handling path is forgetting to DECREF the object.Nope, that doesn't seem to be it. This version prints "refcount: 2" twice, *and* prints a proper "was never awaited" warning:-----import sysasync def open_file(): passasync def main(): open_file_coro = open_file() print("refcount:", sys.getrefcount(open_file_coro)) try: async with open_file_coro: pass except: pass print("refcount:", sys.getrefcount(open_file_coro))coro = main()try: coro.send(None)except: pass | |||
| msg311058 -(view) | Author: Alyssa Coghlan (ncoghlan)*![]() | Date: 2018-01-29 05:36 | |
Looking at the ceval code, I think Yury's theory is plausible, and we may also be leaving the interpreter's internal stack in a dubious state. Things then get cleaned up if you wrap the async with in a try/except or try/finally:==============>>> async def try_main():... try:... async with open_file():... pass... finally:... pass... >>> try_main().send(None)sys:1: RuntimeWarning: coroutine 'open_file' was never awaitedTraceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 3, in try_mainAttributeError: __aexit__==============Unfortunately for that theory, adding braces and "Py_DECREF(POP());" to the relevant error handling branch *doesn't* fix the problem.I also found another way to provoke similar misbehaviour without async with:==========>>> async def open_file():... pass... >>> open_file()<coroutine object open_file at 0x7f92fe19c548>>>> _<coroutine object open_file at 0x7f92fe19c548>>>> 1__main__:1: RuntimeWarning: coroutine 'open_file' was never awaited1>>> open_file()<coroutine object open_file at 0x7f92fe19c548>>>> del _Traceback (most recent call last): File "<stdin>", line 1, in <module>NameError: name '_' is not defined>>> _<coroutine object open_file at 0x7f92fe19c548>>>> 11>>> _1>>> ========== | |||
| msg311060 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 05:51 | |
Changing async def main(): async with open_file(): passto async def main(): c = open_file() async with c: passalso makes it print the warning :)Also I've made a test out of this snippet and running tests in refleak mode shows that there's indeed no refleak here. | |||
| msg311061 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 05:53 | |
The difference between these two functions is two extra opcodes: STORE_FAST/LOAD_FAST before BEFORE_ASYNC_WITH. With them we have a warning. | |||
| msg311062 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 05:55 | |
So refactoring it into "c = open_file(); async with c" just prolongs the life of the coroutine so that it's GCed outside of WITH_CLEANUP_START/WITH_CLEANUP_FINISH block. Something weird is going on in that block. | |||
| msg311064 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 05:57 | |
Ah, we should never really get to WITH_CLEANUP_START; the exception should be raised in BEFORE_ASYNC_WITH | |||
| msg311065 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 06:20 | |
So the problem was that _PyGen_Finalize wasn't issuing any warnings if there's any error set in the current tstate. And in Nathaniel's case, the current error was an AttributeError('__aexit__').This check is weird, because right before raising the warning, we call PyErr_Fetch to temporarily reset the current exception if any, specifically to raise the warning :)The PR just removes the check. Unless I'm missing something this should fix the issue. | |||
| msg311066 -(view) | Author: Nathaniel Smith (njs)*![]() | Date: 2018-01-29 06:21 | |
Well, I feel silly then:bpo-32605 | |||
| msg311071 -(view) | Author: Alyssa Coghlan (ncoghlan)*![]() | Date: 2018-01-29 06:38 | |
Ah, and in my REPL example, the NameError was pending when the internal result storage was getting set back to None.I'm not sure I even knew the "Don't complain when an exception is pending" check existed, so it would have taken me a long time to find that. | |||
| msg311072 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 06:47 | |
I knew about that "if", but it never fully registered to me why it's there and what it is protecting us from ;) So I had to debug half of ceval loop before I stumbled upon it again ;) | |||
| msg311157 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 19:31 | |
New changeset2a2270db9be9bdac5ffd2d50929bf905e7391a06 by Yury Selivanov in branch 'master':bpo-32703: Fix coroutine resource warning in case where there's an error (GH-5410)https://github.com/python/cpython/commit/2a2270db9be9bdac5ffd2d50929bf905e7391a06 | |||
| msg311158 -(view) | Author: Yury Selivanov (yselivanov)*![]() | Date: 2018-01-29 19:32 | |
Merged; closing this issue. | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:57 | admin | set | github: 76884 |
| 2018-01-31 17:10:01 | xgdomingo | set | nosy: +xgdomingo |
| 2018-01-29 19:32:15 | yselivanov | set | status: open -> closed resolution: fixed messages: +msg311158 stage: patch review -> resolved |
| 2018-01-29 19:31:55 | yselivanov | set | messages: +msg311157 |
| 2018-01-29 06:47:49 | yselivanov | set | messages: +msg311072 |
| 2018-01-29 06:38:53 | ncoghlan | set | messages: +msg311071 |
| 2018-01-29 06:21:25 | njs | set | messages: +msg311066 |
| 2018-01-29 06:20:02 | yselivanov | set | messages: +msg311065 |
| 2018-01-29 06:15:49 | yselivanov | set | keywords: +patch stage: patch review pull_requests: +pull_request5243 |
| 2018-01-29 05:57:56 | yselivanov | set | messages: +msg311064 |
| 2018-01-29 05:55:54 | yselivanov | set | messages: +msg311062 |
| 2018-01-29 05:53:27 | yselivanov | set | messages: +msg311061 |
| 2018-01-29 05:51:34 | yselivanov | set | messages: +msg311060 |
| 2018-01-29 05:36:50 | ncoghlan | set | messages: +msg311058 |
| 2018-01-29 04:23:41 | yselivanov | set | nosy: +ncoghlan components: + Interpreter Core, - asyncio |
| 2018-01-29 04:10:22 | njs | set | messages: +msg311053 |
| 2018-01-29 04:06:16 | njs | create | |