Re: [PATCH 01/10] server: allow polling for no events in set_fd_events
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
Remove the fd from the poll list so we don't get POLLHUP/POLLERR messages when we ask for no events
This is wrong, even when selecting for no events you want to receive POLLHUP/POLLERR. If you really don't want any events you have to use -1 to remove the fd from the loop. But in general ignoring POLLHUP/POLLERR is a bug. -- Alexandre Julliard julliard(a)winehq.org
On Tue, Apr 6, 2010 at 1:08 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
Remove the fd from the poll list so we don't get POLLHUP/POLLERR messages when we ask for no events
This is wrong, even when selecting for no events you want to receive POLLHUP/POLLERR. If you really don't want any events you have to use -1 to remove the fd from the loop. But in general ignoring POLLHUP/POLLERR is a bug.
-- Alexandre Julliard julliard(a)winehq.org
Would allowing adding the fd to the poll loop after it has been removed be more sane? Otherwise we have to resort to ugly things like sock_try_event (which only half work). Mike.
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
Would allowing adding the fd to the poll loop after it has been removed be more sane? Otherwise we have to resort to ugly things like sock_try_event (which only half work).
I don't think that makes sense either. The error condition is not going to go away so you can no longer poll on it anyway, you really need to close the socket and create a new one. -- Alexandre Julliard julliard(a)winehq.org
On Tue, Apr 6, 2010 at 2:03 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
Would allowing adding the fd to the poll loop after it has been removed be more sane? Otherwise we have to resort to ugly things like sock_try_event (which only half work).
I don't think that makes sense either. The error condition is not going to go away so you can no longer poll on it anyway, you really need to close the socket and create a new one.
-- Alexandre Julliard julliard(a)winehq.org
That's only true in the case of a full close; if you do shutdown(,SD_SEND) on the remote end, we will get POLLIN/0 recv, but if we decide to do a send after that and the connection gets broken (either the remote fully closes the socket or we get connection reset), we will receive a POLLHUP/POLLERR with a different error code, which is what windows reports. The problem is that there are two halves of the connection to hang up (when both have hangup we get a POLLHUP, otherwise just a POLLIN/0 recv). (data from http://www.greenend.org.uk/rjk/2001/06/poll.html ) But I see your point that perhaps it doesn't belong in the main loop when one of the halves gets closed. I guess we can keep sock_try_event around but only use it when the socket has been removed from the main loop. I'll also add detection of half-closed connection and drop that event if the other half hasn't hung up yet. Sadly looks like MacOS will be left out of that detection since it reports POLLIN|POLLHUP for everything. Does that sound good? Mike.
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
But I see your point that perhaps it doesn't belong in the main loop when one of the halves gets closed. I guess we can keep sock_try_event around but only use it when the socket has been removed from the main loop. I'll also add detection of half-closed connection and drop that event if the other half hasn't hung up yet. Sadly looks like MacOS will be left out of that detection since it reports POLLIN|POLLHUP for everything.
Does that sound good?
I think sock_try_event shouldn't even do a poll() at all. Once we have received POLLHUP there's no point in continuing to poll. All it should have to do is try a MSG_PEEK read to detect EOF for platforms where that triggers a POLLHUP. All other cases should be handled by the main loop. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Apr 8, 2010 at 11:16 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
But I see your point that perhaps it doesn't belong in the main loop when one of the halves gets closed. I guess we can keep sock_try_event around but only use it when the socket has been removed from the main loop. I'll also add detection of half-closed connection and drop that event if the other half hasn't hung up yet. Sadly looks like MacOS will be left out of that detection since it reports POLLIN|POLLHUP for everything.
Does that sound good?
I think sock_try_event shouldn't even do a poll() at all. Once we have received POLLHUP there's no point in continuing to poll. All it should have to do is try a MSG_PEEK read to detect EOF for platforms where that triggers a POLLHUP. All other cases should be handled by the main loop.
-- Alexandre Julliard julliard(a)winehq.org
This won't work for implementing half-closed sockets. If we have only the read half is closed, we will get POLLIN/0 recv indefinitely if we keep polling (not POLLHUP). If we remove it from the loop, we have to keep polling (once in a while, i.e. when an enable_socket_event gets through) until we get a POLLHUP/POLLERR which signals the other half is closed. On some platforms this won't work, but they will jump to a full close automatically (i.e. POLLHUP right away). The problem now is that if we receive a shutdown, we will spam FD_CLOSE on every successful send. I suppose if we don't care to send more than 1 FD_CLOSE (which windows does do), we can just remove the socket from the loop and be done with it. I don't know if any apps actually depend on receiving a second FD_CLOSE after the remote end has sent a shutdown. Should we just implement the 1 FD_CLOSE behavior and then if any regressions relating to it pop up bring back try_event? Mike.
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
This won't work for implementing half-closed sockets. If we have only the read half is closed, we will get POLLIN/0 recv indefinitely if we keep polling (not POLLHUP). If we remove it from the loop, we have to keep polling (once in a while, i.e. when an enable_socket_event gets through) until we get a POLLHUP/POLLERR which signals the other half is closed. On some platforms this won't work, but they will jump to a full close automatically (i.e. POLLHUP right away). The problem now is that if we receive a shutdown, we will spam FD_CLOSE on every successful send.
I don't see why. If you don't get POLLHUP then you can keep polling, just don't select for POLLIN once you have seen EOF. The problem with POLLHUP/POLLERR is that they can't be masked, but POLLIN doesn't have that issue. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Apr 8, 2010 at 8:29 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
This won't work for implementing half-closed sockets. If we have only the read half is closed, we will get POLLIN/0 recv indefinitely if we keep polling (not POLLHUP). If we remove it from the loop, we have to keep polling (once in a while, i.e. when an enable_socket_event gets through) until we get a POLLHUP/POLLERR which signals the other half is closed. On some platforms this won't work, but they will jump to a full close automatically (i.e. POLLHUP right away). The problem now is that if we receive a shutdown, we will spam FD_CLOSE on every successful send.
I don't see why. If you don't get POLLHUP then you can keep polling, just don't select for POLLIN once you have seen EOF. The problem with POLLHUP/POLLERR is that they can't be masked, but POLLIN doesn't have that issue.
-- Alexandre Julliard julliard(a)winehq.org
Shoot, I'm sorry I was confusing poll vs FD_*. You're right of course, that would work. The slight problem is that a 0 recv is not necessarily a full POLLHUP (i.e. both halves closed). So on platforms that don't signal POLLHUP/POLLERR when both halves are closed we're going to sit there and do nothing, after the first half of the socket is closed. (Are there any like this still around?) In other words we can get rid of try_event completely. There is also a small issue that even after a full close, windows will let you call send() and succeed but with an erroneous FD_CLOSE right after. I think the best way to deal with it is just to special case it in enable_socket_event to send an error close without polling anything, if for some reason FD_CLOSE is unheld. I'll send patches on Monday. Mike.
participants (2)
-
Alexandre Julliard -
Mike Kaplinskiy