Skip to content

/ cpython

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-39239: epoll.unregister() no longer ignores EBADF #17882

Merged
merged 1 commit into from Jan 7, 2020
Merged

bpo-39239: epoll.unregister() no longer ignores EBADF #17882

merged 1 commit into from Jan 7, 2020

Conversation

@vstinner
Copy link
Member

vstinner commented Jan 6, 2020 •
edited by bedevere-bot

The select.epoll.unregister() method no longer ignores the EBADF
error.

https://bugs.python.org/issue39239
@vstinner vstinner force-pushed the vstinner:epoll_ebadf branch from bdcbf9e to 2e607ff Jan 6, 2020
@vstinner
bpo-39239: epoll.unregister() no longer ignores EBADF
Loading status checks…
dcf6802 
The select.epoll.unregister() method no longer ignores the EBADF
error.
@vstinner vstinner force-pushed the vstinner:epoll_ebadf branch from 2e607ff to dcf6802 Jan 7, 2020
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Jan 7, 2020

cc @tiran @giampaolo @pitrou
@tiran
tiran approved these changes Jan 7, 2020
View changes
Copy link
Member

tiran left a comment

I don't recall why I ignored EBADF here. Victor has made a compelling argument to stop ignoring the error. I trust his judgement.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Jan 7, 2020

cc @1st1 @asvetlov
@asvetlov
asvetlov approved these changes Jan 7, 2020
View changes
Copy link
Contributor

asvetlov left a comment

I agree with the pull request.
Silent ignoring of an error was a bad idea and looks like oversight.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Jan 7, 2020

@asvetlov: "I agree with the pull request. Silent ignoring of an error was a bad idea and looks like oversight."

Do you expect EBADF errors in legit asyncio code? I don't think so. asyncio is not supposed to unregister a closed FD. I mean: I don't see why I would happen, except if the application does something wrong, no?
@asvetlov

This comment has been minimized.

Sign in to view
Copy link
Contributor

asvetlov commented Jan 7, 2020 •
edited

I don't see why I would happen, except if the application does something wrong, no?

You are correct.
Since the PR is green I assume the change is safe; I don't recall any misusage in asyncio code.
To double-check you can add test-with-buildbot label and see the result; I don't expect a failure though.
@vstinner vstinner merged commit 5b23f76 into python:master Jan 7, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200107.1 succeeded
Details
bedevere/issue-number Issue number 39239 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner vstinner deleted the vstinner:epoll_ebadf branch Jan 7, 2020
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Jan 7, 2020

Thanks for your trust @tiran and @asvetlov ;-)

To double-check you can add test-with-buildbot label and see the result; I don't expect a failure though.

I'm overconfident and merged my PR :-) If I break a buildbot worker, I will fix it ;-)
@giampaolo
giampaolo approved these changes Jan 7, 2020
View changes
Copy link
Contributor

giampaolo left a comment

+1
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Jan 7, 2020

@giampaolo: "+1"

Oh ok, so I wasn't the only one surprised by ignoring silently EBADF ;-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov

@tiran tiran

@giampaolo giampaolo

Assignees
No one assigned
Labels
CLA signed awaiting merge
Projects
None yet
Milestone
No milestone
6 participants
@vstinner @asvetlov @tiran @giampaolo @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.