----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49404/#review140093 -----------------------------------------------------------
Fix it, then Ship it! Great cleanup, thanks! 3rdparty/libprocess/src/process.cpp (line 1392) <https://reviews.apache.org/r/49404/#comment205391> Technically this changes the semantics in that if an old fd is already present we will no longer overwrite but that would have been a bug anyway. Ideally this code checks that the insertion happens: ``` synchronized (mutex) { auto result = sockets.emplace(socket, socket); CHECK(result->second) << "Socket fd already present in map"; } ``` 3rdparty/libprocess/src/process.cpp (line 1556) <https://reviews.apache.org/r/49404/#comment205392> Seems we should check that this inserted: ``` // sockets[s] = socket.get() CHECK(sockets.emplace(s, socket.get())->second); ``` Or less messy: ``` CHECK_EQ(0u, sockets.count(s)); sockets.emplace(s, socket.get()); ``` It's a bit unfortunate that we lost readability. Ideally we could check our invariants here for the other maps as well, feel free to punt since we're only interested in removing the socket pointer mess. 3rdparty/libprocess/src/process.cpp (lines 1580 - 1589) <https://reviews.apache.org/r/49404/#comment205395> I was initially mislead by the comment for `existing` that we made a copy for some kind of lifetime correctness, rather than just readability to passing an argument. Feel free to do this: ``` // Update all the data structures that are mapped to the old // socket. They will now point to the new socket we are about // to try to connect. The old socket should no longer have any // references after the swap and should be closed. Socket existing(sockets.at(persists.at(to.address))); swap_implementing_socket(existing, socket.get()); ``` 3rdparty/libprocess/src/process.cpp (line 2020) <https://reviews.apache.org/r/49404/#comment205396> This should now be `socket.get()` because the intention is to print the fd. This works for you because of the int cast operator, but in the future if we added a different operator for printing sockets, the output here would silently change. 3rdparty/libprocess/src/process.cpp (line 2103) <https://reviews.apache.org/r/49404/#comment205397> Ditto here for `socket.get()`. - Benjamin Mahler On June 30, 2016, 1:11 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49404/ > ----------------------------------------------------------- > > (Updated June 30, 2016, 1:11 a.m.) > > > Review request for mesos, Benjamin Mahler and Artem Harutyunyan. > > > Bugs: MESOS-5748 > https://issues.apache.org/jira/browse/MESOS-5748 > > > Repository: mesos > > > Description > ------- > > `Sockets` is already a reference-counted `shared_ptr` under the covers. > By passing around `Sockets` by value, we avoid potentially deleting > a reference while the same reference is in use by another function. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > 9bae71246e751e491be5a989eea8aca29c9aa751 > > Diff: https://reviews.apache.org/r/49404/diff/ > > > Testing > ------- > > make check (OSX) > > 3rdparty/libprocess/libprocess-tests > --gtest_filter="ProcessRemoteLinkTest.RemoteLink" --gtest_break_on_failure > --gtest_repeat=10000 > > > Thanks, > > Joseph Wu > >
