Skip to content

Conversation

@tiif
Copy link
Member

@tiif tiif commented Nov 13, 2024

I forgot how the implementation works and was wondering how the assumption here is being upheld. So I wrote a test.

// Edge-triggered notification only notify one thread even if there are
// multiple threads block on the same epfd.

This is mainly written for me to verify if it actually works, so it is perfectly ok to not merge this.

@tiif
Copy link
Member Author

tiif commented Nov 14, 2024

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Nov 14, 2024
@oli-obk oli-obk added this pull request to the merge queue Nov 15, 2024
Merged via the queue into rust-lang:master with commit 64a3bb1 Nov 15, 2024
7 checks passed
@tiif tiif deleted the checkepoll branch November 15, 2024 10:50
@RalfJung
Copy link
Member

Don't our existing tests already cover this case? I added a comment along those lines:

// Check that this is indeed using "ET" (edge-trigger) semantics: a second epoll should return nothing.
check_epoll_wait::<8>(epfd, &[]);

The test is not very clear about the fact that that is what it tests, though.

@RalfJung
Copy link
Member

Ah I guess this is a different aspect of "edge semantics".

@tiif
Copy link
Member Author

tiif commented Nov 22, 2024

Ah I guess this is a different aspect of "edge semantics".

Yup it's different. The non-blocking test checks "if no event occurs between two epoll_wait calls, we should not receive any notification during the second epoll_wait". The test in this PR checks "if multiple threads are blocking on the same epoll fd, a single notification on that epoll fd should wake up only one thread (the alternative would be waking up all threads, i.e. thundering herd behaviour )"

@tiif tiif mentioned this pull request Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Waiting for a review to complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants