Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:epoll_ebadf
Jan 7, 2020
Merged

bpo-39239: epoll.unregister() no longer ignores EBADF#17882
vstinner merged 1 commit into
python:masterfrom
vstinner:epoll_ebadf

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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

https://bugs.python.org/issue39239

@vstinner
bpo-39239: epoll.unregister() no longer ignores EBADF
dcf6802 
The select.epoll.unregister() method no longer ignores the EBADF
error.
@vstinner

vstinner commented Jan 7, 2020

Copy link
Copy Markdown
Member Author

cc @tiran @giampaolo @pitrou

tiran
tiran approved these changes Jan 7, 2020
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

vstinner commented Jan 7, 2020

Copy link
Copy Markdown
Member Author

cc @1st1 @asvetlov

asvetlov
asvetlov approved these changes Jan 7, 2020
View reviewed changes

@asvetlov asvetlov 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 agree with the pull request.
Silent ignoring of an error was a bad idea and looks like oversight.

@vstinner

vstinner commented Jan 7, 2020

Copy link
Copy Markdown
Member Author

@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

asvetlov commented Jan 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
@vstinner
vstinner deleted the epoll_ebadf branch January 7, 2020 14:00
@vstinner

vstinner commented Jan 7, 2020

Copy link
Copy Markdown
Member Author

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 reviewed changes

@giampaolo giampaolo 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.

+1

@vstinner

vstinner commented Jan 7, 2020

Copy link
Copy Markdown
Member Author

@giampaolo: "+1"

Oh ok, so I wasn't the only one surprised by ignoring silently EBADF ;-)

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@vstinner @shihai1991
bpo-39239: epoll.unregister() no longer ignores EBADF (pythonGH-17882)
925217b 
The select.epoll.unregister() method no longer ignores the EBADF
error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes
@tiran tiran tiran approved these changes
@giampaolo giampaolo giampaolo approved these changes

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @asvetlov @tiran @giampaolo @the-knights-who-say-ni @bedevere-bot