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

Raise exception with cause exception #915

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

Conversation

CzBiX
Copy link

@CzBiX CzBiX commented May 28, 2024

Summary

In the current logic, the original exception is discarded, which makes it hard to determine the thrown exception more precisely.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Use case

try:
  resp = pool.request('GET', 'https://example.com')
except ConnectError as error:
  # before
  assert error.__cause__ is None
  # after
  assert isinstance(error.__cause__, ssl.SSLError)

@CzBiX CzBiX force-pushed the raise-original-exception branch from 7b2e3c1 to a8ef8d7 Compare May 28, 2024 18:02
@tomchristie
Copy link
Member

Pull requests framed like this create more work for the maintainer than necessary.
An improvement would be to include a concise example of the behavior both before and after the change.
Ie. It'd help more to focus on the behavior rather than the code.

@CzBiX
Copy link
Author

CzBiX commented Jun 10, 2024

hi @tomchristie, here is the updated use case :

try:
  resp = pool.request('GET', 'https://example.com')
except ConnectError as error:
  # before
  assert error.__cause__ is None
  # after
  assert isinstance(error.__cause__, ssl.SSLError)

@tomchristie
Copy link
Member

Can you demo the traceback in both cases?

I'm reluctant to bring this in too easily... raise exc from None was a deliberate design decision, can you link back to the history on it?

@CzBiX
Copy link
Author

CzBiX commented Jun 12, 2024

This change was introduced in the #880. I can't see this change fixing any bugs or causing any bugs though.

From my point of view, it just influence the information used for debug and trace, and this change rather breaks the coherence of traceback. Note the extra "line 216".

File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection_pool.py", line 216, in handle_request
raise exc from None

Here is the traceback before and after my PR:
before:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/interfaces.py", line 43, in request
    response = self.handle_request(request)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection_pool.py", line 216, in handle_request
    raise exc from None
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection_pool.py", line 196, in handle_request
    response = connection.handle_request(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 99, in handle_request
    raise exc
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 76, in handle_request
    stream = self._connect(request)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 154, in _connect
    stream = stream.start_tls(**kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_backends/sync.py", line 152, in start_tls
    with map_exceptions(exc_map):
  File "/usr/local/lib/python3.12/contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "/usr/local/lib/python3.12/site-packages/httpcore/_exceptions.py", line 14, in map_exceptions
    raise to_exc(exc) from exc
httpcore.ConnectError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)

after:

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/httpcore/_exceptions.py", line 10, in map_exceptions
    yield
  File "/usr/local/lib/python3.12/site-packages/httpcore/_backends/sync.py", line 168, in start_tls
    raise exc
  File "/usr/local/lib/python3.12/site-packages/httpcore/_backends/sync.py", line 163, in start_tls
    sock = ssl_context.wrap_socket(
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/ssl.py", line 455, in wrap_socket
    return self.sslsocket_class._create(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/ssl.py", line 1042, in _create
    self.do_handshake()
  File "/usr/local/lib/python3.12/ssl.py", line 1320, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/interfaces.py", line 43, in request
    response = self.handle_request(request)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection_pool.py", line 196, in handle_request
    response = connection.handle_request(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 99, in handle_request
    raise exc
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 76, in handle_request
    stream = self._connect(request)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_sync/connection.py", line 154, in _connect
    stream = stream.start_tls(**kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/httpcore/_backends/sync.py", line 152, in start_tls
    with map_exceptions(exc_map):
  File "/usr/local/lib/python3.12/contextlib.py", line 158, in __exit__
    self.gen.throw(value)
  File "/usr/local/lib/python3.12/site-packages/httpcore/_exceptions.py", line 14, in map_exceptions
    raise to_exc(exc) from exc
httpcore.ConnectError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)

@MarkusSintonen
Copy link
Contributor

This would be great to fix! We have also hit this issue so many times where we dont see anything about the actual cause for different network issues. (We are using Sentry to monitor the errors and we dont see much of details about the problems as httpcore hides these annoyingly.)

We could also enable this linting rule https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/ under [tool.ruff.lint]. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants