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

S113 - Rule Doesn't Take httpx Default Timeout into Account #12210

Closed
fullerzz opened this issue Jul 5, 2024 · 5 comments
Closed

S113 - Rule Doesn't Take httpx Default Timeout into Account #12210

fullerzz opened this issue Jul 5, 2024 · 5 comments
Assignees
Labels
rule Implementing or modifying a lint rule

Comments

@fullerzz
Copy link

fullerzz commented Jul 5, 2024

The S113 rule has the following description:

Checks for uses of the Python requests or httpx module that omit the timeout parameter.

This rule fails to take into account that httpx enforces a default timeout of 5.0 seconds globally.

Omitting the timeout parameter in httpx's top-level API calls still results in a 5 second timeout. Likewise, the same applies to both httpx.Client and httpx.AsyncClient.

httpx Docs Used for Reference

https://www.python-httpx.org/advanced/timeouts/

HTTPX is careful to enforce timeouts everywhere by default.
The default behavior is to raise a TimeoutException after 5 seconds of network inactivity.

client = httpx.Client()  # 5s timeout by default
httpx.get('http://example.com/api/v1/example')  # 5s timeout by default

Timeouts can be explicitely disabled as so:

# Using the top-level API:
httpx.get('http://example.com/api/v1/example', timeout=None)

# Using a client instance:
with httpx.Client() as client:
    client.get("http://example.com/api/v1/example", timeout=None)

Suggestion

Maybe S113 can check for timeout=None in the case of httpx. Or I can just disable S113 if this rule is working as intended.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jul 5, 2024
@charliermarsh
Copy link
Member

I think it makes sense to respect the default and only flag explicit timeout=None, at least based on the above.

@evanrittenhouse
Copy link
Contributor

Feel free to assign this to me, I have a local PR (mostly) working

@charliermarsh
Copy link
Member

Thanks!

@trim21
Copy link
Contributor

trim21 commented Jul 6, 2024

#12213

@trim21
Copy link
Contributor

trim21 commented Jul 7, 2024

#12213 is merged, maybe we can close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

4 participants