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

Preserve default total client timeout value #7275

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

Conversation

wombatonfire
Copy link

@wombatonfire wombatonfire commented May 8, 2023

What do these changes do?

Changed default value for ClientTimeout.total so that the default is always present, even if not explicitly specified.

Are there changes in behavior for the user?

If a user wants to adjust some specific client timeout, i.e. connect or sock_connect, they won't need to explicitly specify total timeout value in order to preserve it.

Related issue number

Closes #7274

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label May 8, 2023
@wombatonfire
Copy link
Author

Not sure if the documentation should be updated. Currently, it does not describe the behavior that this PR is changing, and the new behavior is somewhat expected, IMHO.

@Dreamsorcerer
Copy link
Member

Currently, it does not describe the behavior that this PR is changing

It says it defaults to None:
https://docs.aiohttp.org/en/stable/client_reference.html?highlight=ClientTimeout#clienttimeout

@wombatonfire
Copy link
Author

It says it defaults to None:
https://docs.aiohttp.org/en/stable/client_reference.html?highlight=ClientTimeout#clienttimeout

I guess I need glasses :) Updated.

@@ -134,7 +134,7 @@

@dataclasses.dataclass(frozen=True)
class ClientTimeout:
total: Optional[float] = None
total: Optional[float] = 5 * 60 # 5 minute default timeout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, is this the expected behaviour if a user were to do something like
ClientTimeout(sock_read=600)?

i.e. They set sock_read to 10 mins, but total is still set to 5 mins, so it achieves nothing.

I'm not sure now if having this default value makes things simpler or more complex...
@webknjaz Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve default total timeout when other timeout is changed in ClientSession.__init__()
2 participants