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