[ADD] missing-timeout: Used when a method calls an external request #6780
Conversation
moylop260
force-pushed
the
req-timeout-vx-moy
branch
6 times, most recently
from
fbe6677 to
5900e60
Compare
moylop260
changed the title
Pull Request Test Coverage Report for Build 2545101125
💛 - Coveralls |
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
5900e60 to
7a3181b
Compare
This comment has been minimized.
This comment has been minimized.
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
7a3181b to
6933c24
Compare
This comment has been minimized.
This comment has been minimized.
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
6933c24 to
73c1d0d
Compare
This comment has been minimized.
This comment has been minimized.
|
Thank you for working on this. It seems the primer links are still broken (this is still very new 😄), it works if you remove the |
Pierre-Sassoulas
added
Enhancement ✨
pylint/checkers/variables.py
Outdated
| @@ -463,6 +463,12 @@ def _has_locals_call_after_node(stmt, scope): | |||
| "Used when there is an unbalanced tuple unpacking in assignment", | |||
| {"old_names": [("E0632", "old-unbalanced-tuple-unpacking")]}, | |||
| ), | |||
| "W0635": ( | |||
There was a problem hiding this comment.
I think it should be an extension, but it's very valuable imo (as it prevent hanging indefinitely). Please wait for other opinion before moving everything :) It should be a new checker separated from the variable checker though.
Thank you ! What should I do with the result of pandas and sentry? |
Well nothing, apart from checking that it make sense and that there's no false positive :) |
|
@Pierre-Sassoulas should this MR also include good/bad documentation examples? |
|
Haven't looked at the code, but I wondered what You can tell Requests to stop waiting for a response after a given number of seconds with the timeout parameter. Nearly all production code should use this parameter in nearly all requests. Failure to do so can cause your program to hang indefinitely: I think it makes sense to have this a default checker with some sane defaults (I haven't looked at the current lists). |
This comment has been minimized.
This comment has been minimized.
For |
Yeah, so it's more about finding sane defaults than having this as an extension, right? |
How did you populate the list @moylop260 ? Are all values there reasonable ? |
This lint was born only with Then confirming in the documentation: 3rd party libs
built-in libs
For built-in methods there is a default timeout setting but It is not easy to detect it if it is already defined in the code import socket
socket.getdefaulttimeout() # by default is NoneSo, let me know if we should reduce the list for timeout-methods default in order to be added manually for each project or each project should remove manually these methods if they have considered a global default defined Note: I missed ftp.lib too but I will wait for you reply in order to know if we should consider add more cases by default or less |
This comment has been minimized.
This comment has been minimized.
|
IMHO I fixed all the pending feedback Let me know what should we do here, please |
There was a problem hiding this comment.
Thanks for working on this @moylop260.
I left some feedback.
Where should I install the dependencies
requestsandpyserialin order to get the tests running well for the inferred checks (I have added inrequirements_test_min.txtbut IDK if it is good)?
That's the right place, but I wonder if we should only do requests. See my other comment.
The doc pipeline is failing since that the file reference still doesn't exists in
mainbranch:
Could you open an issue for this. This should be fixed by ignoring those links in linkcheck. We already do so for other types of links.
Could you also add a documentation example like explained in #5953. I think #6874 offers a nice example of the folder structure.
This comment has been minimized.
This comment has been minimized.
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
e631823 to
7cb07c6
Compare
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on sentry:
This comment was generated for commit 7cb07c6 |
|
Thanks for your feedback It is done! |
There was a problem hiding this comment.
This is looking pretty good already, thank you @moylop260 !
| You can add new methods in your Pylint configuration | ||
| using the pylint ``qname`` of the method | ||
|
|
||
| You can get it using the following code: | ||
|
|
||
| .. code:: python | ||
|
|
||
| from astroid import extract_node | ||
| node = extract_node('from requests import get;get()') | ||
| print(next(node.func.infer()).qname()) | ||
|
|
||
| And the library needs to be installed in order to be detected | ||
|
|
||
| You can add the following possible cases ``timeout-methods``: | ||
|
|
||
| * ``http.client.HTTPConnection`` | ||
| * ``http.client.HTTPSConnection`` | ||
| * ``serial.serialcli.Serial`` | ||
| * ``smtplib.SMTP`` | ||
| * ``suds.client.Client`` | ||
| * ``urllib.request.urlopen`` |
There was a problem hiding this comment.
I feel like this is a bit more technical than it might be. pylint user do not need to know about the notion of qname, that's just the actual not aliased (i.e. logical) name of a function, right ? But the function argument needs to be exactly timeout or it won't work.
| You can add new methods in your Pylint configuration | |
| using the pylint ``qname`` of the method | |
| You can get it using the following code: | |
| .. code:: python | |
| from astroid import extract_node | |
| node = extract_node('from requests import get;get()') | |
| print(next(node.func.infer()).qname()) | |
| And the library needs to be installed in order to be detected | |
| You can add the following possible cases ``timeout-methods``: | |
| * ``http.client.HTTPConnection`` | |
| * ``http.client.HTTPSConnection`` | |
| * ``serial.serialcli.Serial`` | |
| * ``smtplib.SMTP`` | |
| * ``suds.client.Client`` | |
| * ``urllib.request.urlopen`` | |
| You can add new methods that should have a defined ```timeout`` argument as qualified names | |
| in the ``timeout-methods`` options, for example: | |
| * ``http.client.HTTPConnection`` | |
| * ``http.client.HTTPSConnection`` | |
| * ``serial.serialcli.Serial`` | |
| * ``smtplib.SMTP`` | |
| * ``suds.client.Client`` | |
| * ``urllib.request.urlopen`` |
There was a problem hiding this comment.
Check the Daniel review where he requested me to add it here, please
There was a problem hiding this comment.
I see a review where he asked to move it to details.rst and I agree, but I'm wondering if we need to bother users with pylint's internal. They could add a value without using astroid, right ?
There was a problem hiding this comment.
No, they don't see the previous discussion about:
There was a problem hiding this comment.
Something to add here?
There was a problem hiding this comment.
Yeah, I think @Pierre-Sassoulas Is right this might be a bit too much for a simple details.rst file. I'm going to commit his changes.
pylint/checkers/method_args.py
Outdated
| for keyword in node.keywords: | ||
| if isinstance(keyword, nodes.Keyword) or keyword.arg == "timeout": | ||
| break | ||
| else: | ||
| self.add_message( | ||
| "missing-timeout", | ||
| node=node, | ||
| args=(node.func.as_string(),), | ||
| confidence=INFERENCE, | ||
| ) |
There was a problem hiding this comment.
This algorithm does not work if we want to check methods args generically like the checker name promise. We probably needs a mapping between keyword args (timeout) target functions ( self.config.timeout_methods) and the corresponding message we'll raise (missing-timeout) so we can add new similar checks.
There was a problem hiding this comment.
Sorry, I don't get you
Could you help me to understand it, please?
There was a problem hiding this comment.
Say we want to catch call of whatever.bar and sentry.something.foo that do not define a foobar argument, and then raise a missing-foobar message, we'd have to refactor the checker for it to work. The checker's current name imply this could be a goal. We could also rename the checker to MissingTimeoutChecker and not handle the generic case, I wouldn't mind, but I think it was nice to identify that it could be generic.
There was a problem hiding this comment.
It is expected
A new checker needs to change a lot of things
One of them is create the new checker number, doc, unittest and change the logic of the method to support it
It can not be configured dynamically since that is not possible to add new checkers dynamically as far I know
There was a problem hiding this comment.
We could add values to check the argument x is defined in functions, f1, f2, f3, by modifying a data structure instead of adding new code. Say something like this:
[
{
"functions": (
"requests.api.delete",
"requests.api.get",
"requests.api.head",
"requests.api.options",
"requests.api.patch",
"requests.api.post",
"requests.api.put",
"requests.api.request",
),
"required_args": "timeout",
},
+ {
+ "functions": (
+ "whatever.bar",
+ "sentry.something.foo",
+ ),
+ "required_args": "foobar",
+ }
]But let's just rename the checker.
There was a problem hiding this comment.
Totally agree
So, Let me know if it requires a change, please
There was a problem hiding this comment.
I do think that the checker class can be generic.
Let's do this in a follow-up. (Not resolving so we can find this conversation easily).
There was a problem hiding this comment.
My bad, I meant checker function. Let's not start adding abstract generic checker classes 😅
There was a problem hiding this comment.
Sorry, I even don't get what should I change here
Could you help me to understand the changes required, please?
There was a problem hiding this comment.
The idea is to have a generic function so we could add data to a structure to check similar cases by modifying the data structure and not adding code (#6780 (comment)). But let's design it and refactor in another PR, this one is already doing a lot.
| You can add new methods in your Pylint configuration | ||
| using the pylint ``qname`` of the method | ||
|
|
||
| You can get it using the following code: | ||
|
|
||
| .. code:: python | ||
|
|
||
| from astroid import extract_node | ||
| node = extract_node('from requests import get;get()') | ||
| print(next(node.func.infer()).qname()) | ||
|
|
||
| And the library needs to be installed in order to be detected | ||
|
|
||
| You can add the following possible cases ``timeout-methods``: | ||
|
|
||
| * ``http.client.HTTPConnection`` | ||
| * ``http.client.HTTPSConnection`` | ||
| * ``serial.serialcli.Serial`` | ||
| * ``smtplib.SMTP`` | ||
| * ``suds.client.Client`` | ||
| * ``urllib.request.urlopen`` |
There was a problem hiding this comment.
Yeah, I think @Pierre-Sassoulas Is right this might be a bit too much for a simple details.rst file. I'm going to commit his changes.
|
@moylop260 Could you give maintainers write acces to this branch? I'd like to merge some of the final changes but I don't have permission. |
I know that option but I even didn't find it in this PR: I have added your gh user in our fork repository explicitly Let me know if you need something else and thanks for take care about this |
|
@moylop260 Thanks! That worked. I think organisations can turn off that function globally, perhaps something to consider as it is quite useful for projects maintainers. Pushed my changes! 😄 |
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on sentry:
This comment was generated for commit 1d6c4d2 |
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on sentry:
This comment was generated for commit 978e145 |
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on sentry:
This comment was generated for commit 2203fe0 |
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
2203fe0 to
1f7cba2
Compare
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on pandas:
Effect on sentry:
This comment was generated for commit 1f7cba2 |
There was a problem hiding this comment.
This is a great new checker, thank you for taking all the remark into account @moylop260 ! I think we can merge.
|
The problem in readthedoc is not that easy to spot it's: |
Calling external request needs to use timeout in order to avoid waiting for a long time
You can even reproduce the case using deelay.me
e.g.
```python
import requests
response = requests.get("https://deelay.me/5000/http://localhost:80") # It will spend 5s
response = requests.get("https://deelay.me/5000/http://localhost:80", timeout=2) # timeout
response = requests.get("https://deelay.me/1000/http://localhost:80", timeout=2) # fine
```
After 2s if the request doesn't have response it raises the following exception
requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='deelay.me', port=443): Read timed out. (read timeout=2)
But if it responses <=1s it is fine
Now you can test the same but using a bigger delay
moylop260
force-pushed
the
req-timeout-vx-moy
branch
from
1f7cba2 to
c4dda29
Compare
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on pandas:
Effect on sentry:
This comment was generated for commit c4dda29 |
Thank you for your patience and depth review I have already fixed the doc error the current one is expected: /home/runner/work/pylint/pylint/doc/user_guide/messages/warning/missing-timeout.rst:38: WARNING: broken link: https://github.com/PyCQA/pylint/blob/main/pylint/checkers/method_args.py (404 Client Error: Not Found for url: https://github.com/PyCQA/pylint/blob/main/pylint/checkers/method_args.py) |
Description
Calling external request needs to use timeout in order to avoid waiting for a long time
You can even reproduce the case using deelay.me
e.g.
After 2s if the request doesn't have response it raises the following exception
requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='deelay.me', port=443): Read timed out. (read timeout=2)
But if it responses <=1s it is fine
Now you can test the same but using a bigger delay
You can define your custom methods using the configuration parameter: --timeout-methods
The default methods are:
- requests.delete
- requests.get
- requests.head
- requests.options
- requests.patch
- requests.post
- requests.put
- requests.request
- serial.Serial
You even could add the following extra methods manually if there is not a default defined:
- http.client.HTTPConnection
- http.client.HTTPSConnection
- smtplib.SMTP
NOTE: The maintainers don't like the idea to add in the doc the following description, so adding it here to remember it myself
Notice the libraries needs to be installed and the name should be the output of pylint infered. " "from astroid import extract_node;" "node = extract_node('from requests import get;get()');" "print(next(node.func.infer()).qname())",Type of Changes