From fd0911cc0fb66a17a1486a27af175109879a8042 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Sat, 17 Nov 2018 22:04:37 -0800 Subject: [PATCH] Guard the service protocol's global handlers list with a reader/writer lock (#6888) (#6895) 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 | 6 +++ fml/BUILD.gn | 8 ++++ fml/platform/posix/shared_mutex_posix.cc | 30 +++++++++++++++ fml/platform/posix/shared_mutex_posix.h | 28 ++++++++++++++ fml/synchronization/atomic_object.h | 36 +++++++++++++++++ fml/synchronization/shared_mutex.h | 49 ++++++++++++++++++++++++ fml/synchronization/shared_mutex_std.cc | 25 ++++++++++++ fml/synchronization/shared_mutex_std.h | 28 ++++++++++++++ runtime/service_protocol.cc | 18 ++++----- runtime/service_protocol.h | 7 ++-- 10 files changed, 223 insertions(+), 12 deletions(-) create mode 100644 fml/platform/posix/shared_mutex_posix.cc create mode 100644 fml/platform/posix/shared_mutex_posix.h create mode 100644 fml/synchronization/atomic_object.h create mode 100644 fml/synchronization/shared_mutex.h create mode 100644 fml/synchronization/shared_mutex_std.cc create mode 100644 fml/synchronization/shared_mutex_std.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 2fc0c7a9f63e5..63b2ac899e260 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -164,6 +164,8 @@ FILE: ../../../flutter/fml/platform/posix/file_posix.cc FILE: ../../../flutter/fml/platform/posix/mapping_posix.cc FILE: ../../../flutter/fml/platform/posix/native_library_posix.cc FILE: ../../../flutter/fml/platform/posix/paths_posix.cc +FILE: ../../../flutter/fml/platform/posix/shared_mutex_posix.cc +FILE: ../../../flutter/fml/platform/posix/shared_mutex_posix.h FILE: ../../../flutter/fml/platform/win/errors_win.cc FILE: ../../../flutter/fml/platform/win/errors_win.h FILE: ../../../flutter/fml/platform/win/file_win.cc @@ -176,9 +178,13 @@ 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 +FILE: ../../../flutter/fml/synchronization/shared_mutex.h +FILE: ../../../flutter/fml/synchronization/shared_mutex_std.cc +FILE: ../../../flutter/fml/synchronization/shared_mutex_std.h FILE: ../../../flutter/fml/synchronization/thread_annotations.h FILE: ../../../flutter/fml/synchronization/thread_annotations_unittest.cc FILE: ../../../flutter/fml/synchronization/thread_checker.h diff --git a/fml/BUILD.gn b/fml/BUILD.gn index f73f7f40714c4..0f1a34dec369d 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -46,8 +46,10 @@ 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/shared_mutex.h", "synchronization/thread_annotations.h", "synchronization/thread_checker.h", "synchronization/waitable_event.cc", @@ -83,6 +85,12 @@ source_set("fml") { libs = [] + if (is_ios || is_mac) { + sources += [ "platform/posix/shared_mutex_posix.cc" ] + } else { + sources += [ "synchronization/shared_mutex_std.cc" ] + } + if (is_ios || is_mac) { sources += [ "platform/darwin/cf_utils.cc", diff --git a/fml/platform/posix/shared_mutex_posix.cc b/fml/platform/posix/shared_mutex_posix.cc new file mode 100644 index 0000000000000..dcbb2938308c5 --- /dev/null +++ b/fml/platform/posix/shared_mutex_posix.cc @@ -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 "flutter/fml/platform/posix/shared_mutex_posix.h" +#include "flutter/fml/logging.h" + +namespace fml { + +SharedMutex* SharedMutex::Create() { + return new SharedMutexPosix(); +} + +SharedMutexPosix::SharedMutexPosix() { + FML_CHECK(pthread_rwlock_init(&rwlock_, nullptr) == 0); +} + +void SharedMutexPosix::Lock() { + pthread_rwlock_wrlock(&rwlock_); +} + +void SharedMutexPosix::LockShared() { + pthread_rwlock_rdlock(&rwlock_); +} + +void SharedMutexPosix::Unlock() { + pthread_rwlock_unlock(&rwlock_); +} + +} // namespace fml diff --git a/fml/platform/posix/shared_mutex_posix.h b/fml/platform/posix/shared_mutex_posix.h new file mode 100644 index 0000000000000..9364400015410 --- /dev/null +++ b/fml/platform/posix/shared_mutex_posix.h @@ -0,0 +1,28 @@ +// 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. + +#ifndef FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_POSIX_H_ +#define FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_POSIX_H_ + +#include +#include "flutter/fml/synchronization/shared_mutex.h" + +namespace fml { + +class SharedMutexPosix : public SharedMutex { + public: + virtual void Lock(); + virtual void LockShared(); + virtual void Unlock(); + + private: + friend SharedMutex* SharedMutex::Create(); + SharedMutexPosix(); + + pthread_rwlock_t rwlock_; +}; + +} // namespace fml + +#endif // FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_POSIX_H_ diff --git a/fml/synchronization/atomic_object.h b/fml/synchronization/atomic_object.h new file mode 100644 index 0000000000000..2a06792ff4a24 --- /dev/null +++ b/fml/synchronization/atomic_object.h @@ -0,0 +1,36 @@ +// 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. + +#ifndef FLUTTER_FML_SYNCHRONIZATION_ATOMIC_OBJECT_H_ +#define FLUTTER_FML_SYNCHRONIZATION_ATOMIC_OBJECT_H_ + +#include + +namespace fml { + +// A wrapper for an object instance that can be read or written atomically. +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 + +#endif // FLUTTER_FML_SYNCHRONIZATION_ATOMIC_OBJECT_H_ diff --git a/fml/synchronization/shared_mutex.h b/fml/synchronization/shared_mutex.h new file mode 100644 index 0000000000000..07acb43461bee --- /dev/null +++ b/fml/synchronization/shared_mutex.h @@ -0,0 +1,49 @@ +// 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. + +#ifndef FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_H_ +#define FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_H_ + +namespace fml { + +// Interface for a reader/writer lock. +class SharedMutex { + public: + static SharedMutex* Create(); + virtual ~SharedMutex() = default; + + virtual void Lock() = 0; + virtual void LockShared() = 0; + virtual void Unlock() = 0; +}; + +// RAII wrapper that does a shared acquire of a SharedMutex. +class SharedLock { + public: + explicit SharedLock(SharedMutex& shared_mutex) : shared_mutex_(shared_mutex) { + shared_mutex_.LockShared(); + } + + ~SharedLock() { shared_mutex_.Unlock(); } + + private: + SharedMutex& shared_mutex_; +}; + +// RAII wrapper that does an exclusive acquire of a SharedMutex. +class UniqueLock { + public: + explicit UniqueLock(SharedMutex& shared_mutex) : shared_mutex_(shared_mutex) { + shared_mutex_.Lock(); + } + + ~UniqueLock() { shared_mutex_.Unlock(); } + + private: + SharedMutex& shared_mutex_; +}; + +} // namespace fml + +#endif // FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_H_ diff --git a/fml/synchronization/shared_mutex_std.cc b/fml/synchronization/shared_mutex_std.cc new file mode 100644 index 0000000000000..12509d6893bf1 --- /dev/null +++ b/fml/synchronization/shared_mutex_std.cc @@ -0,0 +1,25 @@ +// 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 "flutter/fml/synchronization/shared_mutex_std.h" + +namespace fml { + +SharedMutex* SharedMutex::Create() { + return new SharedMutexStd(); +} + +void SharedMutexStd::Lock() { + mutex_.lock(); +} + +void SharedMutexStd::LockShared() { + mutex_.lock_shared(); +} + +void SharedMutexStd::Unlock() { + mutex_.unlock(); +} + +} // namespace fml diff --git a/fml/synchronization/shared_mutex_std.h b/fml/synchronization/shared_mutex_std.h new file mode 100644 index 0000000000000..84ab270bd89ed --- /dev/null +++ b/fml/synchronization/shared_mutex_std.h @@ -0,0 +1,28 @@ +// 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. + +#ifndef FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_STD_H_ +#define FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_STD_H_ + +#include +#include "flutter/fml/synchronization/shared_mutex.h" + +namespace fml { + +class SharedMutexStd : public SharedMutex { + public: + virtual void Lock(); + virtual void LockShared(); + virtual void Unlock(); + + private: + friend SharedMutex* SharedMutex::Create(); + SharedMutexStd() = default; + + std::shared_timed_mutex mutex_; +}; + +} // namespace fml + +#endif // FLUTTER_FML_SYNCHRONIZATION_SHARED_MUTEX_STD_H_ diff --git a/runtime/service_protocol.cc b/runtime/service_protocol.cc index 602ebccf645f2..ea93a120facc7 100644 --- a/runtime/service_protocol.cc +++ b/runtime/service_protocol.cc @@ -45,7 +45,8 @@ ServiceProtocol::ServiceProtocol() kRunInViewExtensionName, kFlushUIThreadTasksExtensionName, kSetAssetBundlePathExtensionName, - }) {} + }), + handlers_mutex_(fml::SharedMutex::Create()) {} ServiceProtocol::~ServiceProtocol() { ToggleHooks(false); @@ -53,21 +54,21 @@ ServiceProtocol::~ServiceProtocol() { void ServiceProtocol::AddHandler(Handler* handler, Handler::Description description) { - std::lock_guard lock(handlers_mutex_); + fml::UniqueLock lock(*handlers_mutex_); handlers_.emplace(handler, description); } void ServiceProtocol::RemoveHandler(Handler* handler) { - std::lock_guard lock(handlers_mutex_); + fml::UniqueLock lock(*handlers_mutex_); handlers_.erase(handler); } void ServiceProtocol::SetHandlerDescription(Handler* handler, Handler::Description description) { - std::lock_guard lock(handlers_mutex_); + fml::SharedLock 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 +176,7 @@ bool ServiceProtocol::HandleMessage(fml::StringView method, return HandleListViewsMethod(response); } - std::lock_guard lock(handlers_mutex_); + fml::SharedLock lock(*handlers_mutex_); if (handlers_.size() == 0) { WriteServerErrorResponse(response, @@ -246,12 +247,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_); + fml::SharedLock 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..1ab1f9183d208 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 "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/shared_mutex.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_; + std::unique_ptr handlers_mutex_; + std::map> handlers_; FML_WARN_UNUSED_RESULT static bool HandleMessage(const char* method,