From 9352360c8bdc14bc746f8db1142925f960cf2c38 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Sat, 17 Nov 2018 10:53:26 -0800 Subject: [PATCH] Guard the service protocol's global handlers list with a reader/writer lock (#6888) The service protocol holds the lock while waiting for completion of service RPC tasks. These tasks (specifically hot restart/RunInView) may need to modify a handler's description data. Task execution and ServiceProtocol::SetHandlerDescription will obtain a shared lock to make this possible. AddHandler and RemoveHandler will obtain an exclusive lock in order to guard against a handler being deleted while a service task is running. --- ci/licenses_golden/licenses_flutter | 1 + fml/BUILD.gn | 1 + fml/synchronization/atomic_object.h | 30 +++++++++++++++++++++++++++++ runtime/service_protocol.cc | 15 +++++++-------- runtime/service_protocol.h | 7 ++++--- 5 files changed, 43 insertions(+), 11 deletions(-) create mode 100644 fml/synchronization/atomic_object.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 2fc0c7a9f63e5..3fb752a6257fa 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -176,6 +176,7 @@ FILE: ../../../flutter/fml/platform/win/wstring_conversion.h FILE: ../../../flutter/fml/string_view.cc FILE: ../../../flutter/fml/string_view.h FILE: ../../../flutter/fml/string_view_unittest.cc +FILE: ../../../flutter/fml/synchronization/atomic_object.h FILE: ../../../flutter/fml/synchronization/count_down_latch.cc FILE: ../../../flutter/fml/synchronization/count_down_latch.h FILE: ../../../flutter/fml/synchronization/count_down_latch_unittests.cc diff --git a/fml/BUILD.gn b/fml/BUILD.gn index f73f7f40714c4..10d8e01c7c397 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -46,6 +46,7 @@ source_set("fml") { "paths.h", "string_view.cc", "string_view.h", + "synchronization/atomic_object.h", "synchronization/count_down_latch.cc", "synchronization/count_down_latch.h", "synchronization/thread_annotations.h", diff --git a/fml/synchronization/atomic_object.h b/fml/synchronization/atomic_object.h new file mode 100644 index 0000000000000..c14af79acfb10 --- /dev/null +++ b/fml/synchronization/atomic_object.h @@ -0,0 +1,30 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +namespace fml { + +template +class AtomicObject { + public: + AtomicObject() = default; + AtomicObject(T object) : object_(object) {} + + T Load() const { + std::lock_guard lock(mutex_); + return object_; + } + + void Store(const T& object) { + std::lock_guard lock(mutex_); + object_ = object; + } + + private: + mutable std::mutex mutex_; + T object_; +}; + +} // namespace fml diff --git a/runtime/service_protocol.cc b/runtime/service_protocol.cc index 602ebccf645f2..449f1b2cd11e8 100644 --- a/runtime/service_protocol.cc +++ b/runtime/service_protocol.cc @@ -53,21 +53,21 @@ ServiceProtocol::~ServiceProtocol() { void ServiceProtocol::AddHandler(Handler* handler, Handler::Description description) { - std::lock_guard lock(handlers_mutex_); + std::unique_lock lock(handlers_mutex_); handlers_.emplace(handler, description); } void ServiceProtocol::RemoveHandler(Handler* handler) { - std::lock_guard lock(handlers_mutex_); + std::unique_lock lock(handlers_mutex_); handlers_.erase(handler); } void ServiceProtocol::SetHandlerDescription(Handler* handler, Handler::Description description) { - std::lock_guard lock(handlers_mutex_); + std::shared_lock lock(handlers_mutex_); auto it = handlers_.find(handler); if (it != handlers_.end()) - it->second = description; + it->second.Store(description); } void ServiceProtocol::ToggleHooks(bool set) { @@ -175,7 +175,7 @@ bool ServiceProtocol::HandleMessage(fml::StringView method, return HandleListViewsMethod(response); } - std::lock_guard lock(handlers_mutex_); + std::shared_lock lock(handlers_mutex_); if (handlers_.size() == 0) { WriteServerErrorResponse(response, @@ -246,12 +246,11 @@ void ServiceProtocol::Handler::Description::Write( bool ServiceProtocol::HandleListViewsMethod( rapidjson::Document& response) const { - // Collect handler descriptions on their respective task runners. - std::lock_guard lock(handlers_mutex_); + std::shared_lock lock(handlers_mutex_); std::vector> descriptions; for (const auto& handler : handlers_) { descriptions.emplace_back(reinterpret_cast(handler.first), - handler.second); + handler.second.Load()); } auto& allocator = response.GetAllocator(); diff --git a/runtime/service_protocol.h b/runtime/service_protocol.h index 401702e5d5be3..5392cae337818 100644 --- a/runtime/service_protocol.h +++ b/runtime/service_protocol.h @@ -6,13 +6,14 @@ #define FLUTTER_RUNTIME_SERVICE_PROTOCOL_H_ #include -#include #include +#include #include #include "flutter/fml/compiler_specific.h" #include "flutter/fml/macros.h" #include "flutter/fml/string_view.h" +#include "flutter/fml/synchronization/atomic_object.h" #include "flutter/fml/synchronization/thread_annotations.h" #include "flutter/fml/task_runner.h" #include "rapidjson/document.h" @@ -72,8 +73,8 @@ class ServiceProtocol { private: const std::set endpoints_; - mutable std::mutex handlers_mutex_; - std::map handlers_; + mutable std::shared_timed_mutex handlers_mutex_; + std::map> handlers_; FML_WARN_UNUSED_RESULT static bool HandleMessage(const char* method,