Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.76%. Comparing base (e0bb05b) to head (421208c).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
+ Coverage   80.75%   80.76%   +0.01%     
==========================================
  Files          24       24              
  Lines        1434     1435       +1     
  Branches      206      206              
==========================================
+ Hits         1158     1159       +1     
  Misses        184      184              
  Partials       92       92              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

For better backward compatibility, an alternative approach could be to introduce a timeout_seconds parameter in the DockerContainer.log() function, defaulting to 300 seconds.

I think this is a better approach as changing the timeout behavior is a breaking change

Hey @bdraco ,

I agree that changing the timeout behavior would be a breaking change. I've updated the code accordingly.

Regarding the parameter design, I believe using timeout: Optional[ClientTimeout] = None might be more appropriate than timeout_seconds = 300. This is because we need to distinguish between HTTP timeout and socket timeout - using just timeout_seconds could be ambiguous and doesn't fully capture the parameter's purpose.

On the downside, while allowing an optional ClientTimeout gives users more fine-grained control, it does create a dependency on aiohttp in the library API, which isn't ideal.

What are your thoughts on this approach? Would you prefer a different solution?

We shouldn't use ClientTimeout directly as an external input since it's an aiohttp object that may change in the future. The fact that aiohttp is used internally is an implementation detail of aiodocker which we don't want to leak to the caller.

We should create a new class to pass the timeout and convert it to an aiohttp ClientTimeout before passing it to aiohttp. This way if aiohttp changes how timeouts are handled in the future it won't be a breaking change for aiodocker as we can adapt it internally and externally callers do not need to make any changes.

The fact that aiohttp is used internally is an implementation detail of aiodocker which we don't want to leak to the caller.

Yes, that was exactly my concern. I'll try to come up with a lightweight Timeout abstraction that can be exposed through the aiodocker public API. This should give users the control they need while maintaining proper encapsulation.

Any update about this?

@FrEaKmAn Yeah, sorry, things have been a little hectic lately and I don't want to rush just for the sake of merging. It's totally on still my radar though. I'll try to get to it soon.

Requirements:

• Support composable, structured timeouts with asyncio.timeout(), instead of individually adding timeout arguments to every API.
• Allow specifying detailed timeouts to differentiate socket connection timeouts, response timeouts, etc. like aiohttp.ClientTimeout.
• Allow separating timeout configurations for streaming results as reported in DockerContainer.log() times out after 300 seconds / 5 minutes of streaming #901.
• Embrace two additional PRs:
  • Allow setting timeout for docker.images.build. #973
  • Add timeout argument for docker.container.commit() method #975

I partially agree with the idea not to expose aiohttp.ClientTimeout in our API, but I think aiodocker is a very thin wrapper around the Docker Daemon's REST API using aiohttp and therefore it is impractical to limit the controllability of aiohttp.ClientSession. aiodocker already exposes the connector and session arguments in Docker.__init__().

NOTE: In most cases, what people want for timeout is "conection timeout" which signifies connection failures to the server, not the "total timeout" which imposes time limits for the entire request-response processing.
Unfortunately, asyncio.timeout() cannot distinguish this, so it should be used for the total timeouts only.

I'd like to redesign the aiodocker timeout interface to:

• Specify the connection timeout that applies to new connections by default when creating Docker instance
  • Note that this timeout applies to new connections, not all individual requests, because a TCP connection may be reused across multiple requests depending on the keepalive option.
  • Users are still able to set the global aiohttp.ClientTimeout if they pass an externally created an aiohttp.ClientSession instance to the session argument.
• Move the responsibility of enforcing "total" timeouts to the caller (e.g., using asyncio.timeout())
  • Users may set individual total-processing timeouts to specific requests (e.g., removing timeout for streaming logs while setting timeouts to other one-off request-response pairs).

Closing in favor of #983.
I will include you in the contributor list as Saulius Adamonis and co-author of the merge commit.