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.

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

Do not use the default timeout when streaming logs #908

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sauladam
Copy link

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

  • 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.

Copy link

codecov bot commented Nov 20, 2024

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.

Additional details and impacted files
@@            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:

@asvetlov asvetlov changed the title Do not use the default timeout when streming logs Do not use the default timeout when streaming logs Nov 20, 2024
@bdraco
Copy link
Member

bdraco commented Nov 20, 2024

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
Copy link
Author

sauladam commented Nov 21, 2024

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
Copy link
Member

bdraco commented Nov 21, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants