mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 57358: Implemented discard behavior in process::Queue.
Date Wed, 15 Mar 2017 22:01:25 GMT

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



Can you split the change to add discard semantics to queue? That patch seems orthogonal and
doesn't need the big description about the socket problem. I think it's a pretty natural change
to describe on its own.


3rdparty/libprocess/include/process/queue.hpp
Lines 80-86 (patched)
<https://reviews.apache.org/r/57358/#comment241381>

    This looks racy:
    
    (1) `put()` hits line 48, found the promise without a discard request.
    
    (2) Caller discards future corresponding to the promise found in (1), executes promise->discard.
    
    (3) `put()` hits line 59 and drops the `t` item on the floor.
    
    Perhaps we could implement the expensive discard approach with a TODO to optimize it,
if it's simpler? Alternatively, we need to lock this section, but we need to make sure we
don't deadlock (will let you think more about that).



3rdparty/libprocess/src/process.cpp
Lines 942-947 (patched)
<https://reviews.apache.org/r/57358/#comment241379>

    Can you comment on who is doing this discard? Or why it surfaces as discarded? The implication
in your comment seems to be that this is the case where libprocess is finalizing? Couldn't
quite follow why that's the case.


- Benjamin Mahler


On March 15, 2017, 6:01 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> -----------------------------------------------------------
> 
> (Updated March 15, 2017, 6:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-6919
>     https://issues.apache.org/jira/browse/MESOS-6919
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit deals with a specific case where the SocketImpl class
> passes a self-reference (shared_ptr) into a Future continuation and
> then discards the original Future. The behavior of `Future::discard`
> is that the `onDiscard` event is only chained to the future immediately
> following the discarded future.  i.e.
> 
> ```
> Future<A> top;
> top
>   .onDiscard([]() { /* This gets executed. */ })
> 
>   .then([](const A&) { return Future<B>(); })
>   .onDiscard([]() { /* This also gets executed. */ })
> 
>   .then([](const B&) { return Future<C>(); })
>   .onDiscard([]() { /* But not this. */ });
> 
> top.discard();
> ```
> 
> When a Future is discarded, we only clear the `onDiscard` callbacks,
> which means that all other continuations will continue to reside in
> memory.  In the case of the `Socket` class, the Libevent SSL socket
> implementation had several levels of continuations:
> 
> ```
> // Each item in this queue is backed by a Promise<>::Future.
> Queue<Future<std::shared_ptr<SocketImpl>>> accept_queue;
> 
> // LibeventSSLSocketImpl::accept.
> accept_queue.get()
>   .then(...)
> 
> // Socket::accept. This continuation holds a self-reference
> // to the `shared_ptr<SocketImpl>`.
>   .then([self](...) {...})
> ```
> 
> Because the second continuation is never discarded nor called, we
> end up with a dangling pointer.  For the `Socket` class, this leads
> to an FD leak.
> 
> This commit fixes the self-reference by implementing discard
> handlers in the libprocess Queue class.  When a waiting `Queue::get`
> is discarded, the Promise backing the `Queue::get` is lazily dropped
> and cleaned up.  Data on the queue will then skip these discarded
> Promises and satisfy the next pending waiter.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/queue.hpp ab08e30df742412f22a06202526f8b55350ed435

>   3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/tests/queue_tests.cpp 95b738133fa50641f8f9b83014837d2808e0e4ff

> 
> 
> Diff: https://reviews.apache.org/r/57358/diff/3/
> 
> 
> Testing
> -------
> 
> cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1
> 
> make libprocess-tests
> 
> 3rdparty/libprocess/src/tests/libprocess-tests --gtest_filter="Scheme/HTTPTest.Endpoints/0"
--gtest_repeat="`ulimit -n`"
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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