-
-
Notifications
You must be signed in to change notification settings - Fork 111
Add timeout support with improved documentation #983
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.
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
==========================================
- Coverage 81.57% 81.54% -0.03%
==========================================
Files 24 24
Lines 1422 1447 +25
Branches 190 194 +4
==========================================
+ Hits 1160 1180 +20
- Misses 186 189 +3
- Partials 76 78 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- `aiodocker.docker.XXXX` -> `aiodocker.{appropriate-module-name}.XXXX`
- Reorganize too deeply nested headers in individual module pages
- Add missing ToC index refs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds timeout support to the aiodocker library by introducing a new Timeout configuration class that can be specified at the Docker client level. The changes standardize timeout handling across the library and deprecate per-API float timeout arguments in favor of using asyncio.timeout() for composable timeouts.
- Introduces
aiodocker.types.Timeoutclass withconnectandtotaltimeout fields - Updates the
Dockerclient constructor to accept atimeoutparameter - Modifies streaming APIs (
DockerLogandStream) to properly handle timeouts by overridingtotalandsock_readtoNone - Standardizes documentation formatting across all module RST files
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| aiodocker/types.py | Adds new Timeout dataclass with conversion to aiohttp.ClientTimeout |
| aiodocker/docker.py | Updates Docker client to accept and use Timeout config, adds deprecation warning for float timeouts |
| aiodocker/stream.py | Updates Stream to use client-level timeout with proper overrides for streaming |
| aiodocker/logs.py | Updates DockerLog to use client-level timeout with proper overrides for streaming |
| aiodocker/containers.py | Adds deprecation warning suppression for wait() API |
| aiodocker/utils.py | Adds httpize() overloads for CIMultiDict support |
| docs/*.rst | Standardizes documentation formatting and fixes module references |
| CHANGES/983.feature | Documents the new timeout feature |
| CONTRIBUTORS.txt | Adds contributors |
Comments suppressed due to low confidence (2)
aiodocker/docker.py:480
- The
_query_chunked_postmethod passes the originalheadersparameter instead of the processed_headersto_query(). This causes the processed headers (including the default Content-Type) to be ignored. Changeheaders=headerstoheaders=_headerson line 480.
headers=headers,
aiodocker/utils.py:155
- The
httpize()function implementation always returns a plain dict (converted = {}), but the overload signatures and return type annotation indicate it should returnCIMultiDict[str]when input isCIMultiDict. The function should preserve the input type and returnCIMultiDictwhen the input isCIMultiDict. Consider checking the input type and constructing the appropriate return type:CIMultiDict(converted)ifisinstance(d, CIMultiDict)elseconverted.
def httpize(
d: Optional[JSONObject | CIMultiDict[str | int | bool]],
) -> Optional[Mapping[str, str] | CIMultiDict[str | int | bool]]:
if d is None:
return None
converted = {}
for k, v in d.items():
if isinstance(v, bool):
v = "1" if v else "0"
if not isinstance(v, str):
v = json.dumps(v)
converted[k] = v
return converted
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@asvetlov @bdraco I followed the concept of separating timeout interface from the underlying aiohttp as discussed in #908 (comment), but still I'm in doutful on doing so because concealing aiohttp seems to have no meaning in aiodocker. There are already many APIs like I'd like to modify the PR to use How about your thoughts? |
|
Since there is no reponse, I'll rewrite it using |
This PR embraces:
docker.images.build. #973by introducing a common
Timeoutconfiguration argument to theDockerinstance constructor, which is converted toaiohttp.ClientTimeoutinternally.Changes
aiodocker.types.Timeoutto have an independent interface toaiohttp.ClientTimeout.to_aiohttp_client_timeout()method to separate from the underlying implementation.aiohttp.ClientTimeoutdirectly...aiodocker.logs.DockerLogandaiodocker.stream.Streamto overridetotal=None,sock_read=Noneafter converting the session-levelaiodocker.types.Timeoutconfig to actualaiohttp.ClientTimeoutinstance viato_aiohttp_client_timeout().attrs.evolve()asaiohttp.ClientTimeoutis an immutable (frozen) dataclass. aiohttp also does the same.aiohttp.ClientTimeoutand/orfloatvalues if the method has an optionaltimeoutargument.Impacts to Existing PRs
aiodocker.stream.Stream.docker.images.build. #973 and Add timeout argument for docker.container.commit() method #975, embrace your API call with theasync with asyncio.timeout(): ...block to ensure structured concurrency and composable timeouts.totaland/orsock_readtimeouts.Warning
Trying to set a float value (total timeout) to the aiodocker APIs having legacy
timeoutarguments will now generate an explicit deprecation warning.Checklist
CONTRIBUTORS.txtchangesfolder