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

process: Wake up read and write on EPOLLERR #2218

Merged

Conversation

kleimkuhler
Copy link
Contributor

Motivation

#2174

On epoll platforms, the read end of a pipe closing is signaled to the write end
through the EPOLLERR event [1]. If readiness is not registered for this
event, it will silently pass through epoll_wait calls.

Additionally, this specific case that EPOLLERR is triggered leaves the write
end of the pipe (parent process) waiting for a wakeup that never occurs.

Solution

Similar to the HUP event on Unix platforms, errors are now always masked
through registrations so that both read and write ends of a connection are made
aware of errors.

In cases where pipes are used and the read end closes, write ends that are
waiting for a wakeup are properly notified and try to write again. This allows
a client to observe BrokenPipe and go through the proper cleanup and/or
restablishment of connection.

Closes #2174

Signed-off-by: Kevin Leimkuhler kevin@kleimkuhler.com

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>
@kleimkuhler kleimkuhler requested a review from a team February 4, 2020 22:37
@kleimkuhler kleimkuhler self-assigned this Feb 4, 2020
Copy link
Member

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me! 👍

However, CI seems to be hanging?

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>
@kleimkuhler
Copy link
Contributor Author

It appears kqueue possibly has different behavior on FreeBSD than MacOS.. unfortunately don't see much online about possible differences. Both MacOS and FreeBSD have the same EVFILT_WRITE documentation about what happens when the reader of a pipe disconnects.

@daniel-emotech
Copy link

@kleimkuhler I don't know if this is of any help: https://stackoverflow.com/questions/49481723/difference-in-kqueue-handling-of-fifos-between-mac-os-and-freebsd

Also, I tried to try this out but I'm using a tonic server and when I switch to your pr branch I get thread 'main' panicked at 'there is no reactor running, must be called from the context of Tokio runtime'. No such error when I use the 0.2.11 version on crates.io.

@kleimkuhler
Copy link
Contributor Author

@daniel-emotech I looked into the SO issue as well as a few others and unfortunately have not found anything helpful. If either of the expected flags were set, there would still be an EVFILT_WRITE filter returned which does not appear to be the case. I don't have a FreeBSD setup to test this on, so I'm just removing the test from that target for now.

If someone comes along with this issue on FreeBSD, then there will at least be the chance to ask them for strace output of what is going on.

@daniel-emotech
Copy link

Awesome, do you think this will make it into the next release? Just asking cause I'm currently using a timeout to detect when this occurs and then just never joining threads it happens to which is less than ideal 😬. The only saving grace is it only happens with a certain type of media file no clients have sent to the API just my own internal testing 👀

@kleimkuhler kleimkuhler merged commit 10f1507 into tokio-rs:master Feb 25, 2020
@kleimkuhler kleimkuhler deleted the kleimkuhler/readiness-on-epollerr branch February 25, 2020 21:27
sthagen added a commit to sthagen/tokio-rs-tokio that referenced this pull request Feb 25, 2020
process: Wake up read and write on `EPOLLERR` (tokio-rs#2218)
@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process labels Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokio::process::ChildStdin cannot detect the termination of the child process on Linux?
4 participants