----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53802/#review160824 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 143) <https://reviews.apache.org/r/53802/#comment232051> I don't see any win in the name change from `recv_callback` to `perform_bev_read`. You also didn't delete the declaration of `recv_callback` above even though you changed the function name below. 3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 155) <https://reviews.apache.org/r/53802/#comment232052> Was your inititon that `received_eof` was going to be protected by `synchronized (bev)`? I want to make sure there isn't a race with the IO thread setting `received_eof` in the `event_callback` and another thread reading and missing `received_eof` in `recv`. If you're confident that the `synchronized (bev)` is a sufficient barrier then let's just document that and maybe even move this up closer to `bufferevent* bev;` and specify that it's protected by `synchronized (bev)` which it gets automatically in any of the callbacks (i.e., `recv_callback`, `send_callback`, `event_callback`). 3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 228) <https://reviews.apache.org/r/53802/#comment232053> One of the rasons I prefer calling this `recv_callback` is that it has symmetry with the others, `send_callback` and `event_callback`. 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 399 - 402) <https://reviews.apache.org/r/53802/#comment232054> This is a tricky comment. Here's my iteration on what you have to try and call out that even though we've received EOF there still might be data left in the buffer: While we've received EOF there is still the possibility that we have data left in the buffer that needs to get drained first. Thus, we pretend as though we've just received data and force a `recv_callback` explicitly which will either fulfill an existing `recv_request` with the data or if there is no data in the buffer it will complete the `recv_request` with a length of zero, representing EOF. - Benjamin Hindman On Dec. 15, 2016, 5:53 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53802/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2016, 5:53 p.m.) > > > Review request for mesos, Benjamin Mahler and Joseph Wu. > > > Bugs: MESOS-6802 > https://issues.apache.org/jira/browse/MESOS-6802 > > > 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 > >
