Project

Profile

Help

Issue #3149

closed

pulpcore.plugin.download.asyncio has surprising timeout handling

Added by gmbnomis over 6 years ago. Updated almost 5 years ago.

Status:
CLOSED - WONTFIX
Priority:
Normal
Assignee:
-
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Master
Platform Release:
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
Yes
Tags:
Sprint:
Quarter:

Description

aiohttp defines two timeouts in the ClientSession read_timeout and conn_timeout. The read_timeout is cumulative for the entire connection, i.e. all data must be read within this timeout.

In case of longer connections used for downloads (i.e. connections using the streaming API to get the data in smaller chunks), this may lead to timeouts on a connection that is perfectly fine, as the timeout is global and not per read operation.

For example:

async def single_download():
    async with aiohttp.ClientSession(read_timeout=3, conn_timeout=3) as session:
        downloader = HttpDownloader('http://127.0.0.1:8000/large_file.txt', session=session)
        return await downloader.run()

loop = asyncio.get_event_loop()
result = loop.run_until_complete(single_download())

Results in:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.6/asyncio/base_events.py", line 467, in run_until_complete
    return future.result()
  File "<stdin>", line 4, in single_download
  File "/home/user/github/pulp-3-from-source/venv/src/pulpcore/plugin/pulpcore/plugin/download/asyncio/base.py", line 50, in wrapper
    raise error
  File "/home/user/github/pulp-3-from-source/venv/src/pulpcore/plugin/pulpcore/plugin/download/asyncio/base.py", line 47, in wrapper
    return await func(downloader)
  File "/home/user/github/pulp-3-from-source/venv/src/pulpcore/plugin/pulpcore/plugin/download/asyncio/http.py", line 130, in run
    to_return = await self._handle_response(response)
  File "/home/user/github/pulp-3-from-source/venv/src/pulpcore/plugin/pulpcore/plugin/download/asyncio/http.py", line 112, in _handle_response
    chunk = await response.content.read(1024 * 1024)
  File "/home/user/github/pulp-3-from-source/venv/lib64/python3.6/site-packages/aiohttp/streams.py", line 607, in read
    return (yield from super().read(n))
  File "/home/user/github/pulp-3-from-source/venv/lib64/python3.6/site-packages/aiohttp/streams.py", line 330, in read
    yield from self._wait('read')
  File "/home/user/github/pulp-3-from-source/venv/lib64/python3.6/site-packages/aiohttp/streams.py", line 259, in _wait
    yield from waiter
  File "/home/user/github/pulp-3-from-source/venv/lib64/python3.6/site-packages/aiohttp/helpers.py", line 727, in __exit__
    raise asyncio.TimeoutError from None
concurrent.futures._base.TimeoutError

after approx. 3 seconds.

One could use a high per connection timeout and an additional timeout for read() for long running connections. Something along these lines:

        async with client.get(download_url, timeout=long_timout) as resp:
            try:
...
                        new_data = await asyncio.wait_for(resp.content.read(self.CHUNK_SIZE),
                                                          per_read_timeout)
...
            except asyncio.TimeoutError:
...

Related issues

Related to Pulp - Issue #3918: DeclarativeVersion cannot sync longer than 5 minutes or a timeout error is raisedCLOSED - CURRENTRELEASEdalleyActions
Actions #1

Updated by dalley over 6 years ago

  • Sprint/Milestone set to 47
  • Triaged changed from No to Yes
Actions #2

Updated by rchan over 6 years ago

  • Sprint/Milestone changed from 47 to 48
Actions #3

Updated by daviddavis over 6 years ago

  • Tags Pulp 3 MVP added
Actions #4

Updated by rchan over 6 years ago

  • Sprint/Milestone changed from 48 to 52
Actions #5

Updated by rchan over 6 years ago

  • Sprint/Milestone changed from 52 to 53
Actions #6

Updated by jortel@redhat.com about 6 years ago

  • Sprint/Milestone changed from 53 to 54
Actions #7

Updated by rchan about 6 years ago

  • Sprint/Milestone changed from 54 to 56
Actions #8

Updated by bmbouter about 6 years ago

  • Sprint set to Sprint 33
Actions #9

Updated by bmbouter about 6 years ago

  • Sprint/Milestone deleted (56)
Actions #10

Updated by milan about 6 years ago

  • Assignee set to milan
Actions #11

Updated by milan about 6 years ago

  • Assignee deleted (milan)
Actions #12

Updated by jortel@redhat.com about 6 years ago

  • Sprint Candidate changed from No to Yes
Actions #13

Updated by jortel@redhat.com about 6 years ago

  • Sprint deleted (Sprint 33)
Actions #14

Updated by amacdona@redhat.com about 6 years ago

  • Tags deleted (Pulp 3 MVP)
Actions #15

Updated by bmbouter over 5 years ago

  • Related to Issue #3918: DeclarativeVersion cannot sync longer than 5 minutes or a timeout error is raised added
Actions #16

Updated by bmbouter over 5 years ago

  • Status changed from NEW to CLOSED - WONTFIX

FYI, In the aiohttp docs now (3.3.2) I see additional options named ['total', 'connect', 'sock_connect', 'sock_read']: https://aiohttp.readthedocs.io/en/stable/client_quickstart.html#timeouts

gmbnomis I can see how the plugin writer can do things that make no sense with those types of options. I think they are aiohttp options, so there isn't much Pulp can do to resolve them. aiohttp could have some sanity validation added though, if it doesn't already do that. I recently filed an issue in upstream aiohttp and it was a good experience. Their tracker is here: https://github.com/aio-libs/aiohttp/issues/

In terms of what Pulp can do for this, I can only see closing this as WONTFIX since we aren't in control of those options. It's good to leave here as a reference though and WONTFIX will do that. gmbnomis, if you have other ideas of what should happen please post. I just don't see another option.

Actions #17

Updated by gmbnomis over 5 years ago

bmbouter This issue is not about preventing plugin writers from doing stupid things (life experience tells me that this would be futile...). It was intended as a kind of "heads up" that aiohttp timeouts seem not to be made for long downloads (at that time at least). And apparently that is what happened in #3918.

According to the documentation you linked, the sock_read timeout seems to implement the timeout implemented by the wait_for in the code I posted above. Thus, there should be no need for this anymore.

Nevertheless, I think Pulp should have sane default timeout definitions in the downloader for the typical use case (in order to ease the life of plugin writers). Probably, a very long or no total timeout. Connection and read timeouts should probably be in the range of seconds (or minutes?). Are there proven nectar default timeout settings you could reuse?

Actions #18

Updated by daviddavis about 5 years ago

  • Sprint/Milestone set to 3.0.0
Actions #19

Updated by bmbouter almost 5 years ago

  • Tags deleted (Pulp 3)

Also available in: Atom PDF