◐ Shell
reader mode source ↗
Skip to content

bpo-41001: Add os.eventfd()#20930

Merged
tiran merged 7 commits into
python:masterfrom
tiran:bpo-41001-eventfd
Nov 13, 2020
Merged

bpo-41001: Add os.eventfd()#20930
tiran merged 7 commits into
python:masterfrom
tiran:bpo-41001-eventfd

Conversation

@tiran

@tiran tiran commented Jun 17, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@tiran tiran added the type-feature A feature request or enhancement label Jun 17, 2020
@tiran tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 2891141 to 307cf27 Compare June 17, 2020 17:28
@aeros

aeros commented Jun 18, 2020

Copy link
Copy Markdown
Contributor

Thanks for working on this, Tiran. It seems like it would be worthwhile to add a brief What's New entry for the addition of os.eventfd().

@aeros aeros requested a review from pitrou June 18, 2020 04:31
@tiran

tiran commented Jun 18, 2020

Copy link
Copy Markdown
Member Author

I have updated the documentation, added a select() test case, and included eventfd_read + eventfd_write helper functions. They are just too useful to omit them.

pitrou
pitrou requested changes Jun 18, 2020
View reviewed changes

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

Hide comment

This is nice, though I suspect eventfd_read does a system call even when uncontended?

@bedevere-bot

bedevere-bot commented Jun 18, 2020

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@tiran

tiran commented Jun 19, 2020

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@tiran tiran force-pushed the bpo-41001-eventfd branch from e95a084 to 4910300 Compare June 19, 2020 09:59
@bedevere-bot

bedevere-bot commented Jun 19, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from pitrou June 19, 2020 10:00
@tiran tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 1c504ff to 5c9d03e Compare June 23, 2020 13:35
aeros
aeros approved these changes Jun 23, 2020
View reviewed changes

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

Hide comment

LGTM (other than a minor suggested change for the whatsnew entry).

As mentioned in a previous review comment, I would prefer to have some mention in the docs of the specific use case for eventfd as a lightweight event notifier/waiter (essentially a brief summary of the "Notes" section from the man page) so that users are aware of its general functionality, even if they may not be already familiar with the system call. Unlike in the earlier years of Python, it's becoming increasingly common for developers to have started with Python before C (which has been my own experience). So, I think it's often quite helpful to at least have a summary of the purpose of the function, instead of assuming the reader is already familiar with it.

But IMO, it's fine to merge as is, that can potentially be considered as a future enhancement to the documentation. The core implementation of the wrapper, helpers, and the tests looks good to me.

(Note that I'm not particularly familiar with the details of implementing a CPython wrapper for a system call, I mainly used the existing members that work with FDs for comparison.)

@tiran tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 50747e8 to 54c49fb Compare June 24, 2020 04:04
@aeros

aeros commented Jun 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@tiran This is more of a question than it is feedback, but is there a particular reason why we don't link to a specific man page for additional information here? I see that it's not done for other similar members that are effectively just wrappers for system calls, but it seems like it would be quite useful for readers in this case (particularly since our documentation of it is just a brief summary). This could very easily be done using the :manpage: inline sphinx role:

:manpage:`eventfd(2)`

17 hidden items Load more…
@pablogsal

pablogsal commented Aug 31, 2020

Copy link
Copy Markdown
Member

I would suggest also to add a test using poll/epoll if available, but I don't feel strongly about it

@tiran tiran force-pushed the bpo-41001-eventfd branch from 54c49fb to bc99dd7 Compare November 13, 2020 10:13
@tiran tiran requested a review from markshannon as a code owner November 13, 2020 10:13
@tiran tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from fcaa02c to 0cf9151 Compare November 13, 2020 10:26
tiran and others added 7 commits November 13, 2020 11:27
@tiran
bpo-41001: Add os.eventfd()
019a9fb
@tiran
Add eventfd_read/write, fix value type …
2732046 
The ``eventfd_read`` and ``eventfd_write`` functions are convenient
wrappers that perform proper read/write and conversion from/to uint64_t.

Signed-off-by: Christian Heimes <christian@python.org>
@tiran
Add eventfd select test case …
c0e9b4b 
Signed-off-by: Christian Heimes <christian@python.org>
@tiran
Adress more code reviews …
9c0dff5 
- initval is a uint32_t
- improve documentation
- add whatsnew

Signed-off-by: Christian Heimes <christian@python.org>
@tiran
Always import select …
4efa39c 
Signed-off-by: Christian Heimes <christian@python.org>
@tiran @aeros
Update Doc/whatsnew/3.10.rst …
f3da4ed 
Co-authored-by: Kyle Stanley <aeros167@gmail.com>
@tiran
Add example and mention man page
9c59140
@tiran tiran force-pushed the bpo-41001-eventfd branch from 0cf9151 to 9c59140 Compare November 13, 2020 10:28
@tiran tiran merged commit cd9fed6 into python:master Nov 13, 2020
@bedevere-bot

bedevere-bot commented Nov 13, 2020

Copy link
Copy Markdown

@tiran: Please replace # with GH- in the commit message next time. Thanks!

@tiran tiran deleted the bpo-41001-eventfd branch November 13, 2020 18:48
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@tiran @aeros @adorilson
bpo-41001: Add os.eventfd() (python#20930) …
eb59e4b 
Co-authored-by: Kyle Stanley <aeros167@gmail.com>
@m-tmatma m-tmatma mentioned this pull request Aug 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giampaolo giampaolo giampaolo left review comments

@pablogsal pablogsal pablogsal left review comments

@pitrou pitrou Awaiting requested review from pitrou

@markshannon markshannon Awaiting requested review from markshannon

+1 more reviewer

@aeros aeros aeros approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tiran @aeros @bedevere-bot @pablogsal @giampaolo @pitrou @the-knights-who-say-ni