> On Aug. 5, 2016, 2:51 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 704-713 > > <https://reviews.apache.org/r/50741/diff/1/?file=1463686#file1463686line704> > > > > Don't we need the same fix in `sendfile`? > > > > It seems a bit odd that we go ahead and write to the buffer event when > > we know that the socket has encountered an error or the receiving side has > > closed, because we've already sent a `Future<size_t>(0)` back to the caller. > > > > How about something like this? > > > > ``` > > // Check if the socket disconnected in the interim > > // (i.e. we received an EOF (receiver closed) or ERROR event). > > // > > // TODO(bmahler): If we receive an EOF because the receiving > > // side only shutdown writes on its socket, we can technically > > // still send data on the socket! > > // See: http://www.unixguide.net/network/socketfaq/2.6.shtml > > > > bool disconnected = false; > > > > synchronized (self->lock) { > > if (self->send_request.get() == nullptr) { > > disconnected = true; > > } > > } > > > > if (!disconnected) { > > int result = bufferevent_write_buffer(self->bev, buffer); > > CHECK_EQ(0, result); > > } > > > > evbuffer_free(buffer); > > ``` > > > > It looks like we treat EOF as the socket no longer being writable, > > which isn't necessarily true! I.e. if the caller shuts down writes it may > > still be interesting in reading: > > http://www.unixguide.net/network/socketfaq/2.6.shtml > > > > How about introducing a TODO for this in the event callback code that > > deals with EOF? > > > > ``` > > // TODO(bmahler): If we receive an EOF because the receiving > > // side only shutdown writes on its socket, we can technically > > // still send data on the socket! > > // See: http://www.unixguide.net/network/socketfaq/2.6.shtml > > ``` > > > > Or just filing a ticket to investigate this further, because libprocess > > makes the same assumption and we've never received a report of clients that > > shutdown their write end early: > > > > https://github.com/apache/mesos/blob/1.0.0/3rdparty/libprocess/src/process.cpp#L692-L697
Indeed, we do need the fix in `sendfile()`. And thanks for the link Re: close/shutdown/EOF on sockets! This makes sense, and I agree we should avoid writing to the bufferevent if we find the send_request is null. I modified the patch with your suggestion, but I left the VLOG call since it seems to me this could be useful verbose output for debugging purposes. I also want to be conservative and avoid adding unnecessary log lines, so feel free to speak up if you disagree :) I created a ticket to investigate our socket EOF semantics, and made reference to it in the comments within this patch. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50741/#review144860 ----------------------------------------------------------- On Aug. 5, 2016, 10:19 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50741/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2016, 10:19 p.m.) > > > Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone. > > > Bugs: MESOS-5986 > https://issues.apache.org/jira/browse/MESOS-5986 > > > Repository: mesos > > > Description > ------- > > The lambda placed on the event loop by the libevent SSL > socket's `send()` method previously used a `CHECK` to > ensure that the socket's `send_request` member was not > `nullptr`. This patch removes this check and replaces it > with a log message, since `send_request` may become > `nullptr` any time the socket receives an EOF or ERROR > event. > > > Diffs > ----- > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp > 97af3c25a350f4490f526e096678bb1eab066174 > > Diff: https://reviews.apache.org/r/50741/diff/ > > > Testing > ------- > > Ran a modified test case repeatedly to see this message printed when an SSL > socket receives an EOF at the appropriate time. > > > Thanks, > > Greg Mann > >
