Made resource provider driver start explicit. The driver for HTTP connections of resource providers is implemented as an actor which takes callbacks. It previously started listening for and handling of events on creation. We typically store drivers as member of resource providers.
This setup is in general problematic since it e.g., becomes impossible to write safe callbacks using internal resource provider state; it was e.g., impossible to write a safe callback using the member driver to send calls. This is due to a race between initialization of the member driver and a newly created driver starting to handle events and triggering callbacks making use of a not yet initialized member driver variable. As a concrete example, with a 'MockResourceProvider' holding a 'std::unique_ptr<Driver>' member tests would regularly hit this race, even though the time window between creating a temporary to assign to the member and the assignment appeared to be small. This patch introduces an explicit 'start' method to the driver which is to be used to explicitly start processing of events by the driver. Review: https://reviews.apache.org/r/64650/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/aaef8563 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/aaef8563 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/aaef8563 Branch: refs/heads/master Commit: aaef8563f0c1c149758975103c7066b7e0e81705 Parents: 66dbf99 Author: Benjamin Bannier <benjamin.bann...@mesosphere.io> Authored: Fri Dec 15 17:55:03 2017 +0100 Committer: Benjamin Bannier <bbann...@apache.org> Committed: Fri Dec 15 17:55:03 2017 +0100 ---------------------------------------------------------------------- include/mesos/v1/resource_provider.hpp | 2 ++ src/resource_provider/driver.cpp | 6 ++++++ src/resource_provider/http_connection.hpp | 12 ++++++------ src/resource_provider/storage/provider.cpp | 2 ++ src/tests/mesos.hpp | 2 ++ 5 files changed, 18 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/aaef8563/include/mesos/v1/resource_provider.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/v1/resource_provider.hpp b/include/mesos/v1/resource_provider.hpp index 006889a..787c619 100644 --- a/include/mesos/v1/resource_provider.hpp +++ b/include/mesos/v1/resource_provider.hpp @@ -84,6 +84,8 @@ public: ~Driver(); + void start() const; + Driver(const Driver& other) = delete; Driver& operator=(const Driver& other) = delete; http://git-wip-us.apache.org/repos/asf/mesos/blob/aaef8563/src/resource_provider/driver.cpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/driver.cpp b/src/resource_provider/driver.cpp index 62c4ca1..70b7e32 100644 --- a/src/resource_provider/driver.cpp +++ b/src/resource_provider/driver.cpp @@ -86,6 +86,12 @@ Future<Nothing> Driver::send(const Call& call) return dispatch(process.get(), &DriverProcess::send, call); } + +void Driver::start() const +{ + return dispatch(process.get(), &DriverProcess::start); +} + } // namespace resource_provider { } // namespace v1 { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/aaef8563/src/resource_provider/http_connection.hpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/http_connection.hpp b/src/resource_provider/http_connection.hpp index 207120a..3d5088d 100644 --- a/src/resource_provider/http_connection.hpp +++ b/src/resource_provider/http_connection.hpp @@ -162,18 +162,18 @@ public: lambda::_1)); } + void start() + { + detection = detector->detect(None()) + .onAny(defer(self(), &Self::detected, lambda::_1)); + } + protected: // Because we're deriving from a templated base class, we have // to explicitly bring these hidden base class names into scope. using process::Process<HttpConnectionProcess<Call, Event>>::self; typedef HttpConnectionProcess<Call, Event> Self; - void initialize() override - { - detection = detector->detect(None()) - .onAny(defer(self(), &Self::detected, lambda::_1)); - } - void finalize() override { disconnect(); http://git-wip-us.apache.org/repos/asf/mesos/blob/aaef8563/src/resource_provider/storage/provider.cpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp index 17acf1b..5e7cb5e 100644 --- a/src/resource_provider/storage/provider.cpp +++ b/src/resource_provider/storage/provider.cpp @@ -647,6 +647,8 @@ Future<Nothing> StorageLocalResourceProviderProcess::recover() }), None())); // TODO(nfnt): Add authentication as part of MESOS-7854. + driver->start(); + return Nothing(); })); } http://git-wip-us.apache.org/repos/asf/mesos/blob/aaef8563/src/tests/mesos.hpp ---------------------------------------------------------------------- diff --git a/src/tests/mesos.hpp b/src/tests/mesos.hpp index 6207d62..41f47cf 100644 --- a/src/tests/mesos.hpp +++ b/src/tests/mesos.hpp @@ -2967,6 +2967,8 @@ public: this, lambda::_1), credential)); + + driver->start(); } void connectedDefault()