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

When catching and converting exceptions, handle connection errors first #469

Merged
merged 2 commits into from Oct 27, 2020

Conversation

sjaensch
Copy link
Contributor

Some HTTP libraries (in my case, aiohttp) have exception hierarchies that themselves use multiple inheritance to make sure the specific exceptions they throw also inherit from a generic timeout exception (here: asyncio.TimeoutError). Additionally, they sometimes have bugs where they don't raise the exact exception type they should be raising in specific circumstances: see aiohttp/client.py.

What I'm proposing here is to just switch around two catch statements: in almost all cases, connection errors are either completely separate exceptions from timeout errors, or they are the more specific exception. So catching connection errors first is the safer thing to do.

This fixes a recent build failure for bravado-asyncio, where new versions of aiohttp again changed things with connection errors. We already have code to disable bravado connection error integration tests on Windows since this was an issue on that platform for quite some time already.

I've verified that this change not only fixes the recent build failure, but also makes the tests work properly on Windows.

@coveralls
Copy link

coveralls commented Oct 25, 2020

Coverage Status

Coverage remained the same at 96.344% when pulling fb8b650 on connection-errors-take-precedence into a7858f6 on master.

@sjaensch sjaensch force-pushed the connection-errors-take-precedence branch from 52992f5 to 99c1d8b Compare October 25, 2020 10:17
@sjaensch sjaensch force-pushed the connection-errors-take-precedence branch from 99c1d8b to fb8b650 Compare October 25, 2020 11:08
@sjaensch
Copy link
Contributor Author

@analogue could you please take a look when you have time? This not only fixes an issue with exception handling when used with bravado-asyncio, but also fixes bravado's build.

Copy link
Contributor

@analogue analogue left a comment

Choose a reason for hiding this comment

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

🚢

@sjaensch sjaensch merged commit 20921da into master Oct 27, 2020
@sjaensch sjaensch deleted the connection-errors-take-precedence branch October 27, 2020 08:27
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.

None yet

3 participants