> On Jan. 8, 2017, 3:22 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, line 143 > > <https://reviews.apache.org/r/53802/diff/10/?file=1581240#file1581240line143> > > > > 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.
I liked the name perform_bev_read because this function is no longer simply a continuation of the libevent callback, it also has a callsite in event_callback. I also find it confusing that the XXX_callback functions all have continuations with the same name in this file. However, you make a good point regarding consistency. We could rename this function `_recv_callback`, which matches our usual pattern for continuations, and I could follow up with another patch to similarly change the names of the relevant `send_callback` and `event_callback` functions. Thoughts? > On Jan. 8, 2017, 3:22 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, line 155 > > <https://reviews.apache.org/r/53802/diff/10/?file=1581240#file1581240line155> > > > > 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`). I originally did have received_eof explicitly protected by synchronized (bev), but as BenM mentioned in our offline discussion we should be safe without this since all accesses of received_eof occur in the event loop. I'll move this declaration and add a comment. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53802/#review160824 ----------------------------------------------------------- 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 > >
