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
urllib.request.urlopen leaks exceptions from socket and httplib.client #57945
Comments
The documentation for urlopen says that it raises URLError on error. I am attaching a test program that demonstrates this bug. |
A patch to fix this issue. Catches exceptions from underlying libraries and reraises them as URLError. I put the class name of the underlying exception in the reason to make it more descriptive. |
I think the general pattern of wrapping exceptions in other exceptions is rather unfortunate. It makes it harder to examine the original problem (for example the |
Antoine, could "raise ... from" be used here? Perhaps also using new subclasses of URLError to allow the exceptions to be caught with finer granularity. That way no information would be lost while at the same time not surprising clients who only catch URLError based on the documentation. At least one exception which I feel must be wrapped is socket.timeout. Since we allow a timeout argument to urlopen, it doesn't make sense for the fact of the timeout to be announced by an exception from another library. |
That's a possible compromise indeed. It's up to Senthil to decide
I agree there is a problem with the documentation announcing that all
You still may be interested to know that the request failed because of a |
I have stumbled upon a wrong impression as well by reading the docs, but usually in the code, I tend to catch the specific Exceptions, like timeout instead or URLError when it is known. I saw some libraries following similar pattern too. But that could be changed, if promise in the docs that URLError exception is raised is corrected. I think, the course of action for this bug could be.
Having any important information in the msg part of the Exception is a bad idea, because it is seldom relied upon and can be changed anytime. Shall come out with a patch. |
There is another patch in duplicate bpo-8823, which has simpler exception wrapping code, but does not add wrapping of reply exceptions. Personally, I wouldn’t endorse any new wrapping HTTP client exceptions, and even if wrapping more socket OS errors would be consistent, it could be a backwards compatibility problem. I think the best way forward is documentation: bpo-25633, bpo-22797. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: