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

Explicitly chain exceptions in HTTPConnectionPool._make_request #2866

Closed
wants to merge 2 commits into from

Conversation

zhanpon
Copy link
Contributor

@zhanpon zhanpon commented Jan 4, 2023

Fix #2313. I didn't search other places for the issue, so there might be other raise statements that can be improved as well.

I skipped a changelog entry to be consistent with #2340.

@zhanpon zhanpon marked this pull request as ready for review January 4, 2023 07:55
@zhanpon
Copy link
Contributor Author

zhanpon commented Jan 10, 2023

@sethmlarson @pquentin Would you mind take a look? This one should be straightforward.

@ecerulm ecerulm added the Skip Changelog Pull requests that don't require a changelog entry label Jan 30, 2024
Copy link
Contributor

@ecerulm ecerulm left a comment

Choose a reason for hiding this comment

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

raise new_e from e is not exactly equivalent to the current raise new_e when e is a BaseSSLError or a CertificateError.

@@ -489,7 +489,7 @@ def _make_request(
new_e, (OSError, NewConnectionError, TimeoutError, SSLError)
) and (conn and conn.proxy and not conn.has_connected_to_proxy):
new_e = _wrap_proxy_error(new_e, conn.proxy.scheme)
raise new_e
raise new_e from e
Copy link
Contributor

Choose a reason for hiding this comment

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

_wrap_proxy_error does already explicitly set the __cause__ and raise new_e from e will overwrite that __cause__

so in the case of e being a CertificateError the current code can produce ProxyError -> SSLError -> CertificateError but with this change it will produce ProxyError -> CertificateError instead.

So I think in this case if we really would like to get the "The above exception was the direct cause of the following exception:" instead of "During handling of the above exception, another exception occurred:" would be better to set new_e.__cause__ explicitly after the SSLError.

Reference https://docs.python.org/3/library/exceptions.html#exception-context

@ecerulm
Copy link
Contributor

ecerulm commented Feb 12, 2024

@zhanpon, I'm closing this issue as per my other comment, feel free to reopen it you disagree.

@ecerulm ecerulm closed this Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Pull requests that don't require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v2] Chaining exceptions
2 participants