Urgent need for advice: POLLHUP and sockets

[Ove Kaaven]

On Wed, 17 Apr 2002, Martin Wilck wrote:

> 1. In the code you're talking about, there is a potential mutual recursion
> because sock_get_poll_events calls sock_reselect, which may in turn call
> sock_get_poll_events.

sock_get_poll_events does not call sock_reselect in the code I'm referring
to, I see no reason it should. Its sole purpose is to return the event
mask that should be polled next, it should not effectuate this mask. Or
are you talking about something else? Hmm, sounds like you may be talking
about sock_poll_event... ok, let's see...

> I understand that infinite recursion is prevented by
> handling the sock->hmask field, but I wonder whether it is necessary at
> all that sock_get_poll_events calls sock_reselect() and not
> only set_select_events(sock_get_poll_events()) ?

The event mask to check for may change during sock_poll_event and it would
be nice to check for those new events immediately. But at the moment I
don't recall any situation where it would be necessary.

> I'd prefer a solution where no call sequence
> sock_get_poll_events->sock_reselect->sock_get_poll_events->... would
> be possible.
> 
> 2. IMO, the code snippet
> 
>         if (event & POLLIN)
>         {
>             char dummy;
>             /* Linux 2.4 doesn't report POLLHUP if only one side of the socket
>              * has been closed, so we need to check for it explicitly here */
>             if (!recv( sock->obj.fd, &dummy, 1, MSG_PEEK )) event = POLLHUP;
>          }
> [...]
> 
>     if (event & (POLLERR|POLLHUP))
>         set_select_events( &sock->obj, -1 );
> 
> is wrong, because an empty read only signals that the upstream connection
> was closed.

Hmm. I don't think I wrote that code piece, so I don't recall its
justification. It may well be that it was ill-conceived (at least I don't
quite like the looks of that code), especially since POLLHUP is the
"problem" we tried to solve with all this insidious code, not its
"solution". There used to be different code to detect a closed socket, but
it seems it was removed for some reason (probably because it had other
issues), and then maybe that thing above was hacked on top because the old
code was no longer there?

> The downstream connection may still be open, i.e. FD_WRITE
> events may well occur. In this case it seems to be wrong to do
> set_select_events( &sock->obj, -1 ) _unless_ the kernel had signalled
> POLLHUP|POLLIN, which means that both sides of the socket have been
> closed. Thus, if we have an empty read but no POLLHUP from the kernel,
> we should signal FD_CLOSE to the app, but not pretend we had got POLLHUP
> as the current code does.
> 
> Right?

Yeah.