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

Only emit UnixReady::hup() from kqueue when EVFILT_READ is set #872

Closed
wants to merge 1 commit into from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Aug 22, 2018

This branch changes the kqueue selector to only set the HUP bit for events whose filter type is EVFILT_READ. This is somewhat cargo-culted from how FreeBSD's Linux compatibility code translates kqueue events to epoll events. This appears to resolve tokio-rs/tokio#449.

I'll admit that I don't entirely trust this change. However, all of mio's tests pass with this change, as do all of tokio's when it depends on a version of mio with this patch. I haven't been able to come up with any other solutions to tokio-rs/tokio#449, so this is the best I've got so far. 🤷‍♀️

Signed-off-by: Eliza Weisman [email protected]

@hawkw hawkw requested a review from carllerche August 22, 2018 23:27
@asomers
Copy link
Collaborator

asomers commented Aug 22, 2018

So what happens if a file descriptor EVFILT_WRITE and it gets HUPed? It looks like with this change, mio will set an error iff fflags != 0. Will that cause a write future to return with an error? I can see two ways for this to go wrong:

  1. How can an application check for HUP without also trying to write to the file descriptor? (I don't even know how to express this with mio, even before your change).
  2. What happens if a file descriptor gets HUPed but fflags == 0? It seems to me like mio would spin the cpu in that case, because the HUP condition would still be present when mio calls kevent again. I suspect that this won't happen for sockets, because you would've run across it already. But it might happen for named pipes, which use HUP a little bit differently than sockets.

@carllerche
Copy link
Member

What are the next steps for this PR?

@hawkw
Copy link
Member Author

hawkw commented Sep 18, 2018

@carllerche

What are the next steps for this PR?

Honestly, I'm not sure. I don't know if this is a viable solution given @asomers' questions and my own concerns, but I don't currently have a deep enough understanding of the problem to determine what a better solution might be.

@@ -307,7 +307,9 @@ impl Events {
}

if e.flags & libc::EV_EOF != 0 {
event::kind_mut(&mut self.events[idx]).insert(UnixReady::hup());
if e.filter == libc::EVFILT_READ as Filter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the if statement should be removed and hup should always be set if EV_EOF is set. From the FreeBSD kqueue manual [1]:

EVFILT_READ:
If the read direction of the socket has shutdown, then the filter also sets EV_EOF in flags, and returns the socket error (if any) in fflags.

EVFILT_WRITE:
The filter will set EV_EOF when the reader disconnects [...]

The way I read that is if either you can't read anymore (read direction was shutdown), or you can't write anymore (reader was disconnected) it sets the EV_EOF flag. The best way we can relay that information currently is via hup readiness.

@carllerche
Copy link
Member

#939 should cover this. Thanks for the initial work 👍

@carllerche carllerche closed this May 15, 2019
@carllerche carllerche deleted the eliza/kqueue-hup branch May 15, 2019 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to handle Writable | Hup?
4 participants