mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Bannier" <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 40266: Libprocess Reinitialization: Cleanup SocketManager along side ProcessManager.
Date Wed, 02 Dec 2015 16:08:30 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message