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

Properly wrap asyncio connect errors #235

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

cdeler
Copy link
Member

@cdeler cdeler commented Nov 13, 2020

Closes encode/httpx#1387

Fixed the problem with asyncio backend, when it incorrectly handles connection timeouts.

@cdeler cdeler force-pushed the fix-problems-with-backends-timeouts branch from 118ec02 to b74beea Compare November 13, 2020 14:31
@elonzh
Copy link

elonzh commented Nov 13, 2020

If we need an ordered dict, why not use OrderedDict?

@cdeler
Copy link
Member Author

cdeler commented Nov 13, 2020

You are right. I forgot that dicts are not ordered by default in 'python3.6'

@cdeler cdeler marked this pull request as draft November 13, 2020 19:25
@cdeler cdeler force-pushed the fix-problems-with-backends-timeouts branch from 819d861 to 60ae602 Compare November 13, 2020 19:53
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Thanks! The fix seems legit.

httpcore/_backends/anyio.py Outdated Show resolved Hide resolved
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Some more thoughts…

tests/utils.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

I do appreciate the extra E2E test here, but to be honest this is a fairly big change footprint — so for the record, I would be very much okay with us merging just the fix in _backends.

tests/async_tests/test_interfaces.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
@cdeler
Copy link
Member Author

cdeler commented Nov 14, 2020

I do appreciate the extra E2E test here, but to be honest this is a fairly big change footprint — so for the record, I would be very much okay with us merging just the fix in _backends.

@florimondmanca you are right. I removed all the test code, since it was a bit flaky

@cdeler cdeler force-pushed the fix-problems-with-backends-timeouts branch from 60ae602 to 7836199 Compare November 14, 2020 18:36
@cdeler cdeler marked this pull request as ready for review November 14, 2020 18:43
@florimondmanca
Copy link
Member

@cdeler Thanks for the update!

But the problem with the exceptions order is still actual for anyio backend in case of tcp & uds connection timeouts

So, do we still need to keep the change to the anyio backend?

@cdeler
Copy link
Member Author

cdeler commented Nov 14, 2020

@florimondmanca I'd extract it into separate PR with the test (moreover I can be wrong)

@cdeler
Copy link
Member Author

cdeler commented Nov 16, 2020

@florimondmanca I added another PR with the anyio changeset and tests: #236

Working on this PR I didn't find another tests verified timeouts on I/O routines

@cdeler cdeler requested review from florimondmanca and a team November 16, 2020 10:07
@florimondmanca florimondmanca changed the title Fix problems with backends timeouts Properly wrap asyncio connect errors Nov 16, 2020
@florimondmanca
Copy link
Member

@cdeler Thanks. I retitled this PR according to what this seems to be doing. :-)

@cdeler cdeler merged commit 76f6e91 into encode:master Nov 17, 2020
@cdeler cdeler deleted the fix-problems-with-backends-timeouts branch November 17, 2020 06:54
@florimondmanca florimondmanca mentioned this pull request Nov 20, 2020
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.

httpx does not wrap asyncio.exceptions.TimeoutError as TimeoutException
3 participants