mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 50741: Replaced CHECK in SSL socket's `send()` with a log message.
Date Fri, 05 Aug 2016 22:23:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message