Fixed a potential race in `Sequence`. Adding items to a sequence is accomplished by dispatching `add()` to the sequence actor. However, this could race with the sequence actor termination. Since the sequence destructor discards the sequence's futures, and callers may have registered callbacks which perform cleanup when the futures are discarded, this race may mean that cleanup logic fails to execute upon sequence destruction.
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 is now guaranteed to happen before actor termination. Also added comments to clarify discard 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/e90ae4d4 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/e90ae4d4 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/e90ae4d4 Branch: refs/heads/master Commit: e90ae4d47b31506daa072f0c19872d2c58a50a38 Parents: 37c477a Author: Meng Zhu <[email protected]> Authored: Thu Apr 5 17:44:27 2018 -0700 Committer: Greg Mann <[email protected]> Committed: Thu Apr 5 17:56:34 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/e90ae4d4/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/e90ae4d4/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();
