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 53802: Eliminated an EOF race condition in libprocess SSL socket.
Date Fri, 09 Dec 2016 19:45:54 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53802/#review158710
-----------------------------------------------------------


Fix it, then Ship it!




Looks good to me. I just added another suggestion to handle BEV_EVENT_READING and BEV_EVENT_WRITING
errors independently in a separate patch.


3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 332 - 334)
<https://reviews.apache.org/r/53802/#comment229509>

    I commented below about the "or it has been discarded" when it comes to the CHECK on the
connect request. As far as I can tell, there is only discard handling for recv.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 336)
<https://reviews.apache.org/r/53802/#comment229506>

    Can you add a TODO here to handle the BEV_EVENT_READING and BEV_EVENT_WRITING errors independently?
Right now we treat a read error as a write error and vice versa, since we treat both in the
same way.
    
    I think we're ok in this patch, you can do this separately. We should also assert that
we don't see BEV_EVENT_TIMEOUT here, since we're not using that functionality.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 340)
<https://reviews.apache.org/r/53802/#comment229504>

    Not yours, but what is a "valid" error?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 388 - 390)
<https://reviews.apache.org/r/53802/#comment229503>

    Any reason this is in the locked section? The implication of it being locked it that there
are accesses off of the event loop thread which does not appear to be the case?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 417)
<https://reviews.apache.org/r/53802/#comment229507>

    The comment above about requests being null seems to suggest that the connect request
could be null if it was discarded? But looking at the code it seems we don't handle discard
for connect requests so this can't be null? Might want to clarify this briefly on this CHECK.


- Benjamin Mahler


On Dec. 9, 2016, 7:10 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 7:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
>     https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>    there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>    EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 57eaf4f607d0628db466cc1a139772eeeaa51136

>   3rdparty/libprocess/src/libevent_ssl_socket.cpp dddd0e292a8b0d470f4e199db08f09a0c863d73c

> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> -------
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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