----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46028/#review138340 -----------------------------------------------------------
Could you split the patch into two? ``` Documented SocketManager::next. Updated a stale comment in SocketManager::close. ``` 3rdparty/libprocess/src/process.cpp (lines 321 - 325) <https://reviews.apache.org/r/46028/#comment203507> How about: ``` // Returns the next message (via an Encoder) to send on the socket, or // nullptr if there are no more outgoing messages. Must be called only // after all previously returned messages have been sent. Ownership of // the Encoder is passed to the caller. ``` I'm not sure how the reader is helped by being informed about the implementation details of touching the internal `outgoing` data structure and cleaning up some state (it's also likely to grow stale if the internal data structures change). 3rdparty/libprocess/src/process.cpp (lines 2026 - 2035) <https://reviews.apache.org/r/46028/#comment203502> Your adjustment looks good, but this comment is still stale in that it says: ``` // Note we // need to do this before we call 'sockets.erase(s)' to avoid // the potential race with the last reference being in // 'sockets'.`" ``` This piece no longer reflects the code accurately (it's from the days when we called `::shutdown` directly because `Socket::shutdown` didn't exist, so we could potentially call `::shutdown` without keeping a `Socket` reference around, in which case we shutdown a stale socket fd!! [1]). It looks as if it can be removed in favor of the comment right below this one: ``` // Hold on to the Socket and remove it from the 'sockets' map so // that in the case where 'shutdown()' ends up calling close the // termination logic is not run twice. ``` However, this comment is confusing. It looks as if it is also stale for the same reason. Now, I'm not sure what the comment is trying to say.. why would shutdown call close, how does this lead to running termination logic twice, and why does calling this before erasing from the map have anything to do with this now that we have `Socket::shutdown`? Can you check with Joris? [1] https://github.com/apache/mesos/blob/0.22.0/3rdparty/libprocess/src/process.cpp#L1772-L1775 - Benjamin Mahler On June 16, 2016, 8:55 a.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46028/ > ----------------------------------------------------------- > > (Updated June 16, 2016, 8:55 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Improved comments in SocketManager::next(). > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > b4cdba6b0cd4f8f373f37118cd2e9d4955f2425a > > Diff: https://reviews.apache.org/r/46028/diff/ > > > Testing > ------- > > > Thanks, > > Neil Conway > >
