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

bpo-39673: Map errno==ETIME to TimeoutError #20253

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented May 20, 2020

Copy link
Contributor

@YoSTEALTH YoSTEALTH left a comment

Doc/library/exceptions.rst needs to be updated as well

Copy link
Contributor

@YoSTEALTH YoSTEALTH left a comment

looks good.

@csabella csabella requested a review from ericvsmith May 23, 2020
Copy link
Member

@ericvsmith ericvsmith left a comment

Is it possible to add a test for this? I realize it might be difficult, so it's not a show stopper, but it would be good to test newly added code. There are a few ETIMEDOUT tests, so maybe they could be used as a guide?

Updated blurb to reflect that it's an OSError that's being mapped to a TimeoutError.
@@ -65,7 +65,7 @@ def test_select_error(self):
+-- NotADirectoryError ENOTDIR
+-- PermissionError EACCES, EPERM
+-- ProcessLookupError ESRCH
+-- TimeoutError ETIMEDOUT
+-- TimeoutError ETIME, ETIMEDOUT
Copy link
Member

@ericvsmith ericvsmith May 26, 2020

Choose a reason for hiding this comment

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

I guess this test is as good as we're going to get.

Copy link
Contributor Author

@ZackerySpytz ZackerySpytz May 26, 2020

Choose a reason for hiding this comment

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

To be honest, I'm having second thoughts w.r.t. this PR (especially after the discussion
on python-dev). I don't know a lot about ETIME. I'm not sure if I should have submitted this PR.

Copy link
Contributor

@YoSTEALTH YoSTEALTH Jun 9, 2020

Choose a reason for hiding this comment

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

@ZackerySpytz No need to worry, its only being added to future version of python. With more and more async style coding being adapted ETIME will be handy to have as TimeoutError. It should have been adapted a long time ago but i suppose no one wanted to bother submitting a PR for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants