Skip to content

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 10, 2020
Merged

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab49120
Fix a stack overflow error for asyncio Task or Future repr().
asvetlov Oct 29, 2020
cff4fdf
Update NEWS
asvetlov Oct 29, 2020
1bd6ca5
Update Misc/NEWS.d/next/Library/2020-10-29-11-17-35.bpo-42183.50ZcIi.rst
asvetlov Oct 30, 2020
d9f44ca
Add a note
asvetlov Oct 30, 2020
6845689
Add comment
asvetlov Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions Lib/asyncio/base_futures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
__all__ = ()

import reprlib
from _thread import get_ident

from . import format_helpers

Expand Down Expand Up @@ -41,6 +42,16 @@ def format_cb(callback):
return f'cb=[{cb}]'


# bpo-42183: _repr_running is needed for repr protection
# when a Future or Task result contains itself directly or indirectly.
# The logic is borrowed from @reprlib.recursive_repr decorator.
# Unfortunately, the direct decorator usage is impossible because of
# AttributeError: '_asyncio.Task' object has no attribute '__module__' error.
#
# After fixing this thing we can return to the decorator based approach.
_repr_running = set()
Copy link
Contributor

@aeros aeros Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a brief comment here to clarify the purpose; e.g. "Used for guarding against recursive reprs from futures" or something along those lines. The name _repr_running isn't overly clear by itself without knowing the context of the internal set in reprlib.recursive_repr().

Copy link
Contributor Author

@asvetlov asvetlov Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add. Plus, I want to describe why reprlib.recursive_repr decorator is not applicable (mine first approach was just applying it).

Copy link
Contributor Author

@asvetlov asvetlov Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that adding _asyncio.Task.__module__ allows us to use reprlib.recursive_repr but it is a more invasive change.



def _future_repr_info(future):
# (Future) -> str
"""helper function for Future.__repr__"""
Expand All @@ -49,9 +60,17 @@ def _future_repr_info(future):
if future._exception is not None:
info.append(f'exception={future._exception!r}')
else:
# use reprlib to limit the length of the output, especially
# for very long strings
result = reprlib.repr(future._result)
key = id(future), get_ident()
if key in _repr_running:
result = '...'
else:
_repr_running.add(key)
try:
# use reprlib to limit the length of the output, especially
# for very long strings
result = reprlib.repr(future._result)
finally:
_repr_running.discard(key)
info.append(f'result={result}')
if future._callbacks:
info.append(_format_callbacks(future._callbacks))
Expand Down
18 changes: 18 additions & 0 deletions Lib/test/test_asyncio/test_futures2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# IsolatedAsyncioTestCase based tests
Copy link
Contributor

@aeros aeros Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the motivation here to start a new set of tests using IsolatedAsyncioTestCase for simplification? If so, is there going to be some effort to gradually migrate the existing test_futures or is it just for new ones?

Copy link
Contributor Author

@asvetlov asvetlov Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the gradual migration of the asyncio test suite would be nice.
I believe IsolatedAsyncioTestCase based tests are much more readable.
On another hand, don't fix if not broken :) Plus tests rewriting takes time with very little effort.
For a new test, I prefer the new approach, definitely.

import asyncio
import unittest


class FutureTests(unittest.IsolatedAsyncioTestCase):
async def test_recursive_repr_for_pending_tasks(self):
# The call crashes if the guard for recursive call
# in base_futures:_future_repr_info is absent
# See Also: https://bugs.python.org/issue42183

async def func():
return asyncio.all_tasks()

# The repr() call should not raise RecursiveError at first.
# The check for returned string is not very reliable but
# exact comparison for the whole string is even weaker.
self.assertIn('...', repr(await asyncio.wait_for(func(), timeout=10)))
Copy link
Contributor

@aeros aeros Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not more strictly test result=...? I think they would be effectively the same in most cases, but during local testing of the PR branch, I noticed some issues with regards to the ellipses colliding/cutting off parts of the repr when using asyncio.gather():

>>> await asyncio.gather(
...     asyncio.wait_for(func(), timeout=10),
...     asyncio.wait_for(func(), timeout=10))
[{<Task finished name='Task-18' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe... result=...>}>}>, <Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>}>}>, <Task finishe...1> result=...>, <Task pending...tures.py:391]>}>, <Task finished name='Task-19' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...esult=...>}>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>}>, <Task pending name='Task-17' coro=<<module>() running at <console>:1> cb=[_chain_future.<locals>._call_set_state() at /home/aeros/repos/cpython/Lib/asyncio/futures.py:391]>, <Task finished name='Task-20' coro=<func() done, defined at <console>:1> result={<Task finishe... result=...>}>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe...1> result=...>}>}, {<Task finished name='Task-19' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...esult=...>}>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>}>, <Task finished name='Task-18' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe... result=...>}>}>, <Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>}>}>, <Task finishe...1> result=...>, <Task pending...tures.py:391]>}>, <Task pending name='Task-17' coro=<<module>() running at <console>:1> cb=[_chain_future.<locals>._call_set_state() at /home/aeros/repos/cpython/Lib/asyncio/futures.py:391]>}]

If it's a bit hard to read, this was the problematic line:

<Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>

That might just be an issue with asyncio.gather() reprs though, so I'm not sure exactly what the fix would be.

Copy link
Contributor Author

@asvetlov asvetlov Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that the test check is the code doesn't raise RecursionError.
Probably I should mention it in a comment for the call.

I'm open to suggestions but not I don't see a better test check.
A comparison of the whole repr() string is weak.
It will be broken on any repr() change, e.g. providing info about an additional task's internal state.

I agree that the formatting is not always tidy. From my understanding, it is the result of two strategies: preventing recursion calls and limiting the repr string size.
Maybe we can improve this but I consider it as a separate issue.

Copy link
Contributor

@aeros aeros Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was to use self.assertIn('result=...', repr(await asyncio.wait_for(func(), timeout=10))) rather than matchiing the whole repr() string, but I agree that the repr formatting is more of a separate issue (and lower priority than what the PR mainly addresses).

4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Library/2020-10-29-11-17-35.bpo-42183.50ZcIi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix a stack overflow error for asyncio Task or Future repr().

The overflow occurs under some circumstances when a Task or Future
recursively returns itself.