mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 50741: Replaced CHECK in SSL socket's `send()` with a log message.
Date Fri, 05 Aug 2016 02:51:11 GMT

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


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