Skip to content

Conversation

@tboy1337
Copy link

Summary

This PR enhances error handling in HTTPAdapter to properly wrap unhandled urllib3 HTTPError exceptions in RequestException, ensuring consistent exception handling across the requests library.

Problem

Previously, when urllib3 raised an HTTPError that wasn't explicitly handled (i.e., not SSLError, ReadTimeoutError, or InvalidHeader), the exception would propagate as-is. This created inconsistent error handling behavior where some urllib3 exceptions would bypass the requests exception hierarchy.

Solution

Modified src/requests/adapters.py:

  • Updated the exception handler in the send() method to wrap unknown HTTPError types in RequestException
  • Improved exception chaining by adding from err to all raised exceptions for better debugging and traceback clarity
  • Renamed the exception variable from e to err for improved code readability

Added comprehensive test coverage in tests/test_adapters.py:

  • Created test_unknown_httperror_wrapped_in_requestexception() to verify that generic urllib3 HTTPError exceptions are properly wrapped
  • Test confirms the exception is catchable as RequestException
  • Test verifies proper exception chaining via __cause__
  • Test validates the request object is properly attached to the exception

Changes

  • Files Modified: 2
  • Lines Added: 46
  • Lines Removed: 8

Technical Details

  • Bug Fix: Addresses unhandled urllib3 HTTPError types
  • Exception chaining: All exceptions now properly chain using from err
  • Backward compatible: No breaking changes to the public API

Testing

✅ New unit test added and passing
✅ Verifies proper exception wrapping behavior
✅ Confirms exception chaining integrity
✅ Validates request object attachment

… in RequestException. Added unit test to verify behavior for unhandled urllib3 HTTPError types.
…pped_in_requestexception. Removed reference to Bug psf#7.
@tboy1337
Copy link
Author

tboy1337 commented Oct 22, 2025

I should add that this issue was found because of I think (I'm not 100% sure because everything was also typed according to mypy rules in the commit) pylint in #7054, if it was pylint that found this issue and it is a legitimate issue then it makes a strong case for the project to start using it imo.

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.

1 participant