Skip to content
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

Jump to bottom

"Timeout context manager should be used inside a task" slightly misleading #10153

Closed
1 task done
GDYendell opened this issue Dec 10, 2024 · 4 comments
Closed
1 task done

"Timeout context manager should be used inside a task" slightly misleading #10153

GDYendell opened this issue Dec 10, 2024 · 4 comments
Labels
enhancement

Comments

@GDYendell
Copy link

GDYendell commented Dec 10, 2024 •
edited
Loading

Describe the bug

This error is also raised when the timer context is used within a task, but that task is on a different event loop to the one the ClientSession was created with because it uses asyncio.current_task(loop=self._loop). Calling asyncio.current_task() would show there is a task, but not on the correct event loop.

To Reproduce

This stripped down example is a bit convoluted, but is the least amount of code I could come up with to reproduce what my application does. It has a long running context and spins up an event loop in another thread, which tries to use the session. Now that I have realised the problem have plans to change this entirely, but I thought it would be worth reporting as I think the error could be clearer.

import asyncio
from threading import Thread

from aiohttp import ClientSession


async def main():
    session = ClientSession()

    async def get():
        async with session.get("http://python.org") as response:
            print("Status:", response.status)
            print("Content-type:", response.headers["content-type"])

            html = await response.text()
            print("Body:", html[:15], "...")

    print("This loop")
    loop = asyncio.get_running_loop()
    loop.call_soon_threadsafe(loop.create_task, get())

    await asyncio.sleep(1)

    loop = asyncio.new_event_loop()
    worker = Thread(target=loop.run_forever)
    worker.start()

    print("Wrong loop")
    loop.call_soon_threadsafe(loop.create_task, get())

    await asyncio.sleep(1)


asyncio.run(main())

Expected behavior

The error could make it clear that this error could also occur if the task was created on the wrong event loop and not just that the timeout manager was not used inside a task. Or possibly it could distinguish the two cases and give different errors.

Logs/tracebacks

This loop
Status: 200
Content-type: text/html; charset=utf-8
Body: <!doctype html> ...
Wrong loop
Task exception was never retrieved
future: <Task finished name='Task-7' coro=<main.<locals>.get() done, defined at /workspaces/fastcs-eiger/app.py:10> exception=RuntimeError('Timeout context manager should be used inside a task')>
Traceback (most recent call last):
  File "/workspaces/fastcs-eiger/app.py", line 11, in get
    async with session.get("http://python.org") as response:
  File "/venv/lib/python3.11/site-packages/aiohttp/client.py", line 1423, in __aenter__
    self._resp: _RetType = await self._coro
                           ^^^^^^^^^^^^^^^^
  File "/venv/lib/python3.11/site-packages/aiohttp/client.py", line 607, in _request
    with timer:
  File "/venv/lib/python3.11/site-packages/aiohttp/helpers.py", line 636, in __enter__
    raise RuntimeError("Timeout context manager should be used inside a task")
RuntimeError: Timeout context manager should be used inside a task
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7efc90fd9f50>
Unclosed connector
connections: ['deque([(<aiohttp.client_proto.ResponseHandler object at 0x7efc90e08910>, 115561.676192066)])']
connector: <aiohttp.connector.TCPConnector object at 0x7efc90e02110>

Python Version

$ python --version
Python 3.11.9

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.11.10
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache-2.0
Location: /venv/lib/python3.11/site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, propcache, yarl
Required-by: fastcs-eiger, tickit

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /venv/lib/python3.11/site-packages
Requires: 
Required-by: aiohttp, yarl

propcache Version

$ python -m pip show propcache
Name: propcache
Version: 0.2.1
Summary: Accelerated property cache
Home-page: https://github.com/aio-libs/propcache
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /venv/lib/python3.11/site-packages
Requires: 
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.18.3
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /venv/lib/python3.11/site-packages
Requires: idna, multidict, propcache
Required-by: aiohttp

OS

RHEL8

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@GDYendell GDYendell added the bug label Dec 10, 2024
@Dreamsorcerer
Copy link
Member

I feel like running multiple event loops is not a typical or recommended practice, so if you are doing this, you'd probably need to be aware of these sort of issues anyway. If you have a suggestion for a better message that won't confuse typical users with a single loop (and possibly not even knowing what the loop is), then feel free to open a PR changing it.

@GDYendell
Copy link
Author

Yeah, it's a tricky one. If you want to keep the message simple for the common case, distinct errors would probably be best:

        task = asyncio.current_task(loop=self._loop)
        if task is None:
            if asyncio.current_task() is not None:
                raise RuntimeError("Timeout context manager used in a task running on a different event loop")
            else:
                raise RuntimeError("Timeout context manager should be used inside a task")

Then the error mentioning event loops shouldn't occur if there is only a single event loop.

If you think this is reasonable I can put in a PR, but also I understand that this may not be worth changing.

@Dreamsorcerer
Copy link
Member

I think that looks reasonable. It shouldn't impact performance as the extra check is only done when we're already in an error state.

@GDYendell GDYendell mentioned this issue Dec 11, 2024
Closed
5 tasks
@GDYendell
Copy link
Author

This turned out to be too complicated to implement to be worth it for fixing such a minor problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add distinction between error cases in TimerContext GDYendell/aiohttp
2 participants
@Dreamsorcerer @GDYendell