----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40266/#review108650 -----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 461) <https://reviews.apache.org/r/40266/#comment168102> While using a raw pointer here lets you not worry about `socket_mutex`' lifetime, it does create a future false positive for any leak checks. Instead you should really try to be explicit. You could e.g., use a `shared_ptr<recursive_mutex>`, and, if you feel this adds too much noise to the call sites, pass that one to the callbacks as an implicit parameter, i.e. declare void finalize(shared_ptr<recursive_mutex> m = process::socket_mutex); and similarly for `internal::on_accept`. 3rdparty/libprocess/src/process.cpp (line 493) <https://reviews.apache.org/r/40266/#comment168104> You should be more explicit about lifetimes here and use `unique_ptrs` of .. instead (you can always `reset` in place of `delete` if you need to destroy at a certain point). 3rdparty/libprocess/src/process.cpp (line 993) <https://reviews.apache.org/r/40266/#comment168107> Once you use a proper smart pointer for `process_route` this comment will be right (maybe `s/deleted/cleaned up/`). 3rdparty/libprocess/src/process.cpp (line 2200) <https://reviews.apache.org/r/40266/#comment168109> I find adding another manual iteration index manipulation here makes this even harder to read (e.g., do we really iterate over all elements? Are there assumptions about ordering (hopefully not)?, ...). You could e.g., factor out a `synchronized` helper to get the next not-ignored element (or a `nullptr` if nothing is left); the whole existing loop could then collapse to while (true) { ProcessBase* process = next_cleanup(processes)); if (!process) { break; } process::terminate(process, false); process::wait(process); } This would also make it clear that we intent over all not-ignored processes (which currently is implicit through `wait` removing processes one after the other, and `processes` not containing any `nullptr` elements). The helper `next_cleanup` can be implemented without querying size information, e.g., it could iterate `processes` until the process doesn't pattern-match with ignored processes. - Benjamin Bannier On Nov. 20, 2015, 7:19 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40266/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2015, 7:19 p.m.) > > > Review request for mesos, Artem Harutyunyan and Joris Van Remoortere. > > > Bugs: MESOS-3910 > https://issues.apache.org/jira/browse/MESOS-3910 > > > Repository: mesos > > > Description > ------- > > The `SocketManager` and `ProcessManager` are highly inter-dependent, which > requires some untangling in `process::finalize`. > > * Logic originally found in `~ProcessManager` has been split into > `ProcessManager::finalize` due to what happens during cleanup. > * Some additional cleanup was added for dangling pointers. > * The future from `__s__->accept()` must be explicitly discarded as libevent > does not detect a locally closed socket. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > 8fa0594b671969c2713d1b361f2dbbb07d25b3a8 > > Diff: https://reviews.apache.org/r/40266/diff/ > > > Testing > ------- > > `make check` (libev) > `make check` (--enable-libevent --enable-ssl) > > > Thanks, > > Joseph Wu > >
