Do not use the default timeout when streaming logs

[sauladam]

What do these changes do?

This update modifies the default session timeout in the aiohttp client when calling DockerContainer.log(). Currently, the default timeout appears to be 300 seconds. By explicitly setting a ClientTimeout with sock_read=None, the logs can continue streaming as long as the connection remains active. This seems like a more suitable default behavior for continuous log streaming.

Are there changes in behavior for the user?

There may be a small impact for users who have implemented workarounds to handle the current 5-minute timeout, as they might expect the connection to terminate after that period. However, the overall effect should be minimal.

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

Related issue number

This issue was originally discussed in #901, and the solution was proposed by @toerb. I have adapted this approach and successfully tested it in my own projects.

Checklist

• [x] I think the code is well written
• I’m not sure how to unit test or classify this change, as it’s neither a bug fix nor a new feature - just a default setting that seems more appropriate for this use case.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

---

🚨 Try these New Features:

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

[bdraco]

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

[sauladam]

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?

[bdraco]

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.

[sauladam]

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.