----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50741/#review144860 -----------------------------------------------------------
Thanks for the fix Greg! Shouldn't we apply the same fix to sendfile? Also made a minor suggestion below. 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 704 - 713) <https://reviews.apache.org/r/50741/#comment210991> 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 - Benjamin Mahler On Aug. 4, 2016, 9:57 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50741/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2016, 9:57 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 > >
