Skip to content

Repository navigation

Properly wrap asyncio connect errors - #235

Merged
cdeler merged 1 commit into
encode:masterfrom
cdeler:fix-problems-with-backends-timeouts
Nov 17, 2020
Merged

Properly wrap asyncio connect errors#235
cdeler merged 1 commit into
encode:masterfrom
cdeler:fix-problems-with-backends-timeouts

Conversation

@cdeler

@cdeler cdeler commented Nov 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

elonzh commented Nov 13, 2020

Copy link
Copy Markdown

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

@cdeler

cdeler commented Nov 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

elonzh
elonzh reviewed Nov 13, 2020
View reviewed changes
Comment thread tests/async_tests/test_interfaces.py Outdated
elonzh
elonzh reviewed Nov 13, 2020
View reviewed changes
Comment thread tests/sync_tests/test_interfaces.py Outdated
@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
florimondmanca
florimondmanca suggested changes Nov 14, 2020
View reviewed changes

@florimondmanca florimondmanca left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Comment thread httpcore/_backends/anyio.py Outdated
florimondmanca
florimondmanca reviewed Nov 14, 2020
View reviewed changes

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some more thoughts…

Comment thread tests/utils.py Outdated
Comment thread tests/conftest.py Outdated
Comment thread tests/conftest.py Outdated
florimondmanca
florimondmanca reviewed Nov 14, 2020
View reviewed changes

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread tests/async_tests/test_interfaces.py Outdated
Comment thread tests/utils.py Outdated
@cdeler

cdeler commented Nov 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

florimondmanca commented Nov 14, 2020

Copy link
Copy Markdown
Contributor

@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

cdeler commented Nov 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@cdeler

cdeler commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

@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 a team and florimondmanca November 16, 2020 10:07
@florimondmanca florimondmanca changed the title Fix problems with backends timeouts Properly wrap asyncio connect errors Nov 16, 2020
@florimondmanca

florimondmanca commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

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

@cdeler cdeler mentioned this pull request Nov 16, 2020
Merged
@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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@elonzh elonzh elonzh left review comments
@florimondmanca florimondmanca florimondmanca approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

httpx does not wrap asyncio.exceptions.TimeoutError as TimeoutException

3 participants

@cdeler @elonzh @florimondmanca