Fixed a potential race in `Sequence`. Adding item to sequence is realized by dispatching `add()` to the sequence actor. However, this could race with the sequence actor termination.
This patch fixes this by enqueueing the terminate message at the end of the message queue. Also removed the clock settle in the test `DiscardAll`. As the processing of the messages are now guaranteed to happen before the actor termination. Also added comments to clarify the onDiscard propagation. Review: https://reviews.apache.org/r/66322/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/2d075e3e Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/2d075e3e Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/2d075e3e Branch: refs/heads/1.5.x Commit: 2d075e3ee17d6220e73dd37969d12e9bda537346 Parents: a9cb8d8 Author: Meng Zhu <m...@mesosphere.io> Authored: Wed Apr 4 16:36:50 2018 -0700 Committer: Greg Mann <gregorywm...@gmail.com> Committed: Fri Apr 6 23:42:39 2018 -0700 ---------------------------------------------------------------------- 3rdparty/libprocess/include/process/sequence.hpp | 15 ++++++++++++++- 3rdparty/libprocess/src/tests/sequence_tests.cpp | 6 ------ 2 files changed, 14 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/2d075e3e/3rdparty/libprocess/include/process/sequence.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/sequence.hpp b/3rdparty/libprocess/include/process/sequence.hpp index b4d7593..24712b1 100644 --- a/3rdparty/libprocess/include/process/sequence.hpp +++ b/3rdparty/libprocess/include/process/sequence.hpp @@ -113,6 +113,15 @@ public: // discarded. We use weak futures here to avoid cyclic dependencies. // Discard the future associated with this notifier. + // + // NOTE: When we discard the notifier future, any `onDiscard()` callbacks + // registered on `promise->future` will be invoked, but `onDiscard` + // callbacks registered on the future returned by `add()` will NOT be + // invoked. This is because currently discards do not propagate through + // `dispatch()`. In other words, users should be careful when registering + // `onDiscard` callbacks on the returned future. + // + // TODO(*): Propagate `onDiscard` through `dispatch`. notifier->future().onDiscard( lambda::bind( &internal::discard<T>, @@ -175,7 +184,11 @@ inline Sequence::Sequence(const std::string& id) inline Sequence::~Sequence() { - process::terminate(process); + // We set `inject` to false so that the terminate message is added to the + // end of the sequence actor message queue. This guarantees that all `add()` + // calls which happened before the sequence destruction are processed. + // See MESOS-8741. + process::terminate(process, false); process::wait(process); delete process; } http://git-wip-us.apache.org/repos/asf/mesos/blob/2d075e3e/3rdparty/libprocess/src/tests/sequence_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/sequence_tests.cpp b/3rdparty/libprocess/src/tests/sequence_tests.cpp index 43911b6..3c80a1d 100644 --- a/3rdparty/libprocess/src/tests/sequence_tests.cpp +++ b/3rdparty/libprocess/src/tests/sequence_tests.cpp @@ -195,12 +195,6 @@ TEST(SequenceTest, DiscardAll) EXPECT_CALL(process, func3()) .Times(0); - // Flush the event queue to make sure that all callbacks have been - // added to the sequence. - Clock::pause(); - Clock::settle(); - Clock::resume(); - // This should cancel all pending callbacks. sequence.reset();
