> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote: > > 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?
Thanks for the review, Ben! I've addressed the comments below, and I'll submit follow-up patches for your two points above, as well as for the calls to `EVUTIL_SOCKET_ERROR` in `accept_SSL_callback`. > On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 366-367 > > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line366> > > > > 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). I checked out the windows code in libevent and calling `evutil_socket_error_to_string(0)` looks safe: ``` const char * evutil_socket_error_to_string(int errcode) { /* XXXX Is there really no built-in function to do this? */ int i; for (i=0; windows_socket_errors[i].code >= 0; ++i) { if (errcode == windows_socket_errors[i].code) return windows_socket_errors[i].msg; } return strerror(errcode); } ``` > On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 417-425 > > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line417> > > > > 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). OK, I removed the null checks. As we discovered yesterday in the libevent documentation, writing to the buffer before the connection is established is actually allowed: "It is okay to add data to the output buffer before the connect is done." (http://www.wangafu.net/~nickm/libevent-book/Ref6_bufferevent.html) > On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 367 > > <https://reviews.apache.org/r/53802/diff/6/?file=1573360#file1573360line367> > > > > 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. Since I've removed the `events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0` check, this function has only a single invocation of `EVUTIL_SOCKET_ERROR`. For this reason, I moved the `if` branch which calls `EVUTIL_SOCKET_ERROR` up to the top so that we invoke the macro ASAP without the need to store it in a local variable. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53802/#review158604 ----------------------------------------------------------- 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 > >
