Skip to content

Conversation

ymilki
Copy link
Member

@ymilki ymilki commented May 6, 2025

Bug

In py3.11, concurrent.futures.TimeoutError is aliased to TimeoutError. This is a problem for bravado.http_future._raise_error which constructs a dynamic error to combine the Bravado specific error with the original error. With TimeoutError and BravadoTimeoutError, this will now throw an MRO error:

E       TypeError: Cannot create a consistent method resolution
E       order (MRO) for bases TimeoutError, BravadoTimeoutError

This issue occurs with bravado-asyncio which explicitly throws concurrent.futures.TimeoutError.

Solution

_raise_error was created for python2 to ensure the original exception was not lost. With python 3, we can directly throw BravadoTimeoutError without losing the original exception.

Checking for the specific case where the caught exception is TimeoutError, we will raise BravadoTimeoutError

TODO: Drop py2. This test will fail under py2 because TimeoutError doesn't exist until py33.

Validation

bravady-asyncio has tests that will fail under py311: tests/integration/bravado_integration_test.py::TestServerBravadoAsyncioClient. This PR will make those tests pass.

TODO: Write similar tests in this repo.

@ymilki ymilki requested a review from Copilot May 6, 2025 19:02
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an MRO error that arises in Python 3.11 when a TimeoutError is raised by explicitly checking for it in the _raise_timeout_error method and then raising BravadoTimeoutError.

  • Adds an explicit check for TimeoutError in _raise_timeout_error
  • Raises BravadoTimeoutError directly when the caught exception is a TimeoutError

Co-authored-by: Copilot <[email protected]>
@benbariteau benbariteau mentioned this pull request May 7, 2025
@benbariteau benbariteau marked this pull request as ready for review May 7, 2025 20:22
@benbariteau benbariteau merged commit 0a6b710 into master May 7, 2025
8 checks passed
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.

2 participants