----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53802/#review158604 -----------------------------------------------------------
I went over this with Greg. This looks good, some suggestions around some of the existing logic that we should fix. Also, some other items: (1) It looks like the shutdown override is no longer overriding. The code in shutdown() looks unnecessary? Do we need to clear buffers or something? (2) It looks like we don't need the SSL_set_shutdown workaround code since we don't use BEV_OPT_CLOSE_ON_FREE? 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 250 - 252) <https://reviews.apache.org/r/53802/#comment229385> This seems a bit misleading since this is necessary for both callers? Maybe just avoid this comment? 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 260 - 276) <https://reviews.apache.org/r/53802/#comment229384> Do we still need this comment in the buffer_length > 0 case? Seems inconsistent with the long comment above? ``` if (buffer_length > 0) { size_t length = bufferevent_read(bev, request->data, request->size); CHECK(length > 0); request->promise.set(length); } else { CHECK(received_eof); request->promise.set(0); } ``` 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 281 - 292) <https://reviews.apache.org/r/53802/#comment229389> Looks like we can consolidate `recv_callback()` and `perform_bev_read()`? I'd be inclined to remove this `recv_callback()` in favor of calling `perform_bev_read()` from our call-sites and the `recv_callback(bufferevent*, void*)` implementation. 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 362 - 363) <https://reviews.apache.org/r/53802/#comment229382> Let's remove this part of the case: ``` events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0) ``` and take the CHECK out of the final error case. We can just call evutil_socket_error_to_string even if it's 0 AFAICT (although you should check on windows if it's ok). 3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 363) <https://reviews.apache.org/r/53802/#comment229381> It's not clear if this case is possible. At least looking at the documentation and some of the example code it seems to suggest that we are expected to see a non-zero EVUTIL_SOCKET_ERROR whenever BEV_EVENT_ERROR is set. This is because EVUTIL_SOCKET_ERROR just returns errno (or WSAGetLastError on windows) and unless libevent is clearing these it wouldn't be safe to look at errno since it might still be set to a stale value. Also, libevent says that EVUTIL_SOCKET_ERROR isn't idempotent on all platforms so we should pull this out into a variable. 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 402 - 410) <https://reviews.apache.org/r/53802/#comment229380> Should we also pull out the CHECKs for recv and send requests being null? It seems we actually allow the caller to do this, and the idea being CHECKs is generally to check our internal invariants. So the caller could induce a CHECK failure here by calling send() without waiting for a connect to finish. Unfortunately, I'm not sure how libevent handles the case where the socket is not connected and we write to its buffer (hopefully that would give us a EVUTIL_SOCKET_ERROR back in the event_callback). 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 699 - 709) <https://reviews.apache.org/r/53802/#comment229390> We could probably do away with the comment here, should we call `perform_bev_read()` instead of the callback? - Benjamin Mahler On Nov. 30, 2016, 11:12 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53802/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2016, 11:12 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 > >
