Don't watch for EPOLLRDHUP in epoll

[behrat]

Currently, EPOLLRDHUP sets UnixReady::hup(). This is incorrect behavior
because higher-level libraries like tokio (correctly) assume that
UnixReady::hup() is unclearable since it signals that both the read and
write halfs are shutdown. In reality, EPOLLRDHUP only means that the TCP
stream has been half-closed and such a half-closed stream can still be
written to.

This will fix a current issue with tokio, which is that tokio infinitely
attempts to write to a half-closed socket that's returning WouldBlock
when it's write buffer is full, an issue which manifests with excessive
CPU usage. I think this may help some of the issues discussed in
tokio-rs/tokio#449

After this change, EOF will still be propagated correctly, because
read-hangups also trigger read-readiness via EPOLLIN. However, if
handling of EPOLLRDHUP is desired to be retained, I believe it should be
implemented as another readiness kind on UnixReady, perhaps
UnixReady::read_hup().

Possible concern of a breaking change: Since it's not currently possible
for a user of mio to differentiate between EPOLLHUP and EPOLLRDHUP, it
must be that no users of mio currently are. There may be applications
that test the "health" of a socket by checking for UnixRead::hup(),
which would previously trigger on EPOLLRDHUP but will no longer with
this change. This will change such applications from considering a
half-closed connection as closed to considering it open. However, I
still beleive this change is a correction of the semantics of HUP and
the desired behavior such applications was already ambiguous.

Note: if this is an agreed upon change for epoll, I think a similar
change is in order for kqueue. I think this would be to only set
UnixReady::hup() if (e.filter == libc::EVFILT_READ && e.flags &
libc::EV_EOF != 0), but I will leave that change to someone more
knowledgeable with kqueue.

[behrat]

Points I'd like to open for discussion:

• Is my above analysis correct?
• If so, does my proposal for adding UnixReady::read_hup() make sense?
• Should a test case be added for this, and how? The best case I can think of is something like "test that half-closed socket doesn't set UnixReady::hup()", which I expect would (and should) currently fail on kqueue platforms.

[carllerche]

Sorry for the delay, my kid had chicken pox last week and am still trying to catch up on GitHub notifications.

[behrat]

No problem, of course; that's certainly more important. By the way, we're using this patch in production now, and it seems to have solved the issue, so it's not really urgent for us. We're just trying to contribute back. :)

[carllerche]

Thanks for the thorough analysis. I believe you are correct and this change should be applied.

I do not believe it would count as a breaking change. As you mentioned, EPOLLRDHUP cannot currently be requested. Also, a half shutdown cannot currently be observed in a portable way and spurious events are permitted. If this change breaks any existing code, odds are the code was not originally portable.

Before merging, I will take a look at kqueue. Also, a test would be appreciated 👍

PS. very nice meetup talk.

[carllerche]

I also did a search through the history for any reason indicating why the code is as it is, and it could not find anything.

[carllerche]

@asomers Do you know if this bug exists on the kqueue side of things? It looks like it might.

[carllerche]

I believe, w/ kqueue, we should only set HUP when EV_EOF is received with the EVFILT_WRITE filter. We can experiment more once we have a test in place.

[asomers]

@carllerche I think you're correct about kqueue.

[carllerche]

@asomers looking more at it, it seems that there is no way to detect that a socket fully hung up (both read and write are shutdown) with kqueue unless additional state is tracked... However, the mio docs state:

A HUP (or hang-up) signifies that a stream socket peer closed the connection, or shut down the writing half of the connection.

which we can implement, but I don't know if it is correct...

[carllerche]

It seems like what we need to do in 0.6 is ensure that HUP / RDHUP sets the appropriate readable / writable readiness and rethink the entire thing in 0.7.

This test should expose the problem: #939

[carllerche]

Thanks for doing all the research / work. I'm going to continue this in #939 to get it merged.