> On March 15, 2017, 3:01 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/include/process/queue.hpp > > Lines 80-86 (patched) > > <https://reviews.apache.org/r/57358/diff/3/?file=1665030#file1665030line84> > > > > 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).
I've implemented the expensive-discard. And I think it covers both racing and deadlock cases. To mitigate races, all operations on the internal queues happen inside the synchronized sections. To mitigate deadlocks, any setting/discarding of Promises happens outside the synchronized sections. > On March 15, 2017, 3:01 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/process.cpp > > Lines 942-947 (patched) > > <https://reviews.apache.org/r/57358/diff/3/?file=1665031#file1665031line942> > > > > 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. Moved to https://reviews.apache.org/r/57821/ - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57358/#review169065 ----------------------------------------------------------- On March 21, 2017, 2:40 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57358/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 2:40 p.m.) > > > Review request for mesos, Benjamin Mahler and Greg Mann. > > > Bugs: MESOS-6919 > https://issues.apache.org/jira/browse/MESOS-6919 > > > Repository: mesos > > > Description > ------- > > When a consumer calls `Queue::get` on an empty Queue, the Queue creates > a Promise to back the Future return value. This Promise currently > does not have a discard handler, which may cause problems if the > Future is chained to multiple continuations. For example, see > the scenario in MESOS-6919. > > This commit implements an (potentially expensive) discard handler > on the Queue's Promise. If the Future return value is discarded, > the Queue will remove the corresponding Promise from its internal > queue of promises. The operation is expensive because it needs > to reconstruct the entire internal queue of promises. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/queue.hpp > ab08e30df742412f22a06202526f8b55350ed435 > 3rdparty/libprocess/src/tests/queue_tests.cpp > 95b738133fa50641f8f9b83014837d2808e0e4ff > > > Diff: https://reviews.apache.org/r/57358/diff/4/ > > > 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 > >
