----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64911/#review195406 -----------------------------------------------------------
Refactoring looks good, but I was curious about a change below that seems to alter the semantics of RECONNECT and introduces a race where the first socket wins and the RECONNECT loses. I'm also a little puzzled still at what is preventing two concurrent send loops on a socket. 3rdparty/libprocess/src/process.cpp Line 1677 (original), 1555 (patched) <https://reviews.apache.org/r/64911/#comment274572> newline? 3rdparty/libprocess/src/process.cpp Lines 1677-1683 (original), 1555-1562 (patched) <https://reviews.apache.org/r/64911/#comment274573> Just to clarify, this is an existing issue with the old code as well, right? 3rdparty/libprocess/src/process.cpp Lines 1571-1575 (patched) <https://reviews.apache.org/r/64911/#comment274574> It might not be necessary, but it does seem helpful to spell out that this can happen. 3rdparty/libprocess/src/process.cpp Lines 1657-1659 (patched) <https://reviews.apache.org/r/64911/#comment274575> Why did you need this? It looks like you're already capturing with = in recover so you could look at `socket` anyway? 3rdparty/libprocess/src/process.cpp Lines 1758-1761 (original), 1669-1672 (patched) <https://reviews.apache.org/r/64911/#comment274576> Shouldn't we be CHECKing here that socket.kind is SSL? And maybe also that the future isFailed? 3rdparty/libprocess/src/process.cpp Lines 1697-1703 (patched) <https://reviews.apache.org/r/64911/#comment274577> It looks to me like you need to do this in the mutex? Otherwise it seems like the semantics are changing: Before: FWICT, a RECONNECT will always replace previous socket that's getting connected. After: FWICT, we check here under the mutex that socket is still contained, then RECONNECT swaps it, then we swap it again here on the first connect socket. So the first socket can win the race? 3rdparty/libprocess/src/process.cpp Lines 1805-1822 (original), 1714-1738 (patched) <https://reviews.apache.org/r/64911/#comment274578> Hm.. I was expecting `connect` to return once connected, but this .then returning the receive loop makes it look like `connect` will return after the receive loop finishes reading..? Am I mis-reading this? 3rdparty/libprocess/src/socket_manager.hpp Lines 53-54 (patched) <https://reviews.apache.org/r/64911/#comment274571> Hm.. this could use some more detail about what a downgrade is and when we downgrade? E.g. ``` Helper to connect the socket to the provided address. If the initial connection attempt was done over SSL and fails, we will attempt to "downgrade" the connection by re-connecting without SSL. The "downgrade" can be enabled or disabled via a flag / environment variable. ``` - Benjamin Mahler On Jan. 3, 2018, 7:23 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64911/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2018, 7:23 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Refactored connection logic in libprocess. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf > 3rdparty/libprocess/src/socket_manager.hpp > dd4d111c4ae579420060e547d1111d12f8f0711c > > > Diff: https://reviews.apache.org/r/64911/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
