mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.
Date Tue, 09 Aug 2016 01:17:07 GMT


> On Aug. 4, 2016, 4:48 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1113
> > <https://reviews.apache.org/r/40266/diff/5/?file=1458242#file1458242line1113>
> >
> >     Remove `gc` from this list, since it now gets deleted in the destructor

This entire comment block is actually superceded by one that is added in this review.  I'll
delete the old comment.


> On Aug. 4, 2016, 4:48 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 1116-1118
> > <https://reviews.apache.org/r/40266/diff/5/?file=1458242#file1458242line1116>
> >
> >     It's not clear to me precisely which logic this comment refers to - HTTP authentication
logic occurs in `ProcessBase` rather than `ProcessManager`, so perhaps it's a typo. AFAIK
all HTTP authentication happens within libprocess processes, so terminating all processes
should be sufficient to complete any pending authentication requests and exit safely, but
perhaps it's worth checking with Alexander to see exactly what he meant here?

The authentication manager was not considered in this review because it didn't exist before
:D

The codepath for auth roughly follows:
1) Something connects to the OS process via HTTP.
2) `ProcessManager::handle` is called.  This delegates the request to the appropriate process
and enqueues an `HttpEvent`.  It also creates an `HttpProxy` process.
3) `ProcessBase::visit(const HttpEvent&)` will call `AuthenticatorManager::authenticate`
for some routes.
4) `AuthenticatorManager` will dispatch to a `AuthenticatorManagerProcess`, which dispatches
to some other processes (i.e. `BasicAuthenticatorProcess`).
5) The Authenticator returns a future, which is chained to some `AuthorizationCallbacks` (literally
a bunch of lambdas).

(3) dereferences `authenticator_manager` from inside a `ProcessBase`.  During finalization,
we will have deleted the `AuthenticatorManagerProcess`, which means that dispatches will go
into the ether (the associated futures are never satisfied nor discarded).  

This means, we can safely delete `authenticator_manager` after closing the server socket (no
further `HttpEvent`s).  We must delete it before `process_manager` because the `authenticator_manager`
dereferences `process_manager`.


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40266/#review144841
-----------------------------------------------------------


On July 29, 2016, 4:53 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> -----------------------------------------------------------
> 
> (Updated July 29, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Joris Van Remoortere, and Vinod
Kone.
> 
> 
> 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.
> * The future from `__s__->accept()` must be explicitly discarded as 
>   libevent does not detect a locally closed socket.
> * Terminating `HttpProxy`s must close the associated socket.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/process.cpp 7f331b812de2f0437838f48e0959441c8e04c358 
> 
> 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