diff --git a/cpp/platform/public/BUILD b/cpp/platform/public/BUILD index 714b4859..459584d5 100644 --- a/cpp/platform/public/BUILD +++ b/cpp/platform/public/BUILD @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +load("//tools/build_defs/cc:cc_fake_binary.bzl", "cc_fake_binary") + cc_library( name = "types", srcs = [ @@ -28,6 +30,7 @@ cc_library( "crypto.h", "file.h", "future.h", + "lockable.h", "logging.h", "multi_thread_executor.h", "mutex.h", @@ -38,6 +41,8 @@ cc_library( "single_thread_executor.h", "submittable_executor.h", "system_clock.h", + "thread_check_callable.h", + "thread_check_runnable.h", ], visibility = [ "//googlemac/iPhone/Shared/Nearby/Connections:__subpackages__", @@ -141,3 +146,27 @@ cc_test( "//absl/time", ], ) + +cc_fake_binary( + name = "thread_check_nocompile", + srcs = ["thread_check_nocompile.cc"], + deps = [ + ":types", + "//absl/time", + ], +) + +py_test( + name = "thread_check_nocompile_test", + size = "large", + srcs = ["thread_check_nocompile_test.py"], + data = ["thread_check_nocompile"], + python_version = "PY3", + srcs_version = "PY3", + tags = ["non_compile_test"], + deps = [ + "//pyglib/flags", + "//testing/pybase", + "//testing/pybase:fake_target_util", + ], +) diff --git a/cpp/platform/public/lockable.h b/cpp/platform/public/lockable.h new file mode 100644 index 00000000..73d45721 --- /dev/null +++ b/cpp/platform/public/lockable.h @@ -0,0 +1,51 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef PLATFORM_PUBLIC_LOCKABLE_H_ +#define PLATFORM_PUBLIC_LOCKABLE_H_ + +#include "absl/base/thread_annotations.h" + +namespace location { +namespace nearby { + +// A resource that can be locked. This class is provided +// for clang thread safety analysis at compile time. +// There is no actual locking at runtime. +class ABSL_LOCKABLE Lockable { + private: + void Acquire() const ABSL_EXCLUSIVE_LOCK_FUNCTION() {} + void Release() const ABSL_UNLOCK_FUNCTION() {} + friend class ThreadLockHolder; +}; + +// RAII holder for a Lockable resource. +class ABSL_SCOPED_LOCKABLE ThreadLockHolder { + public: + explicit ThreadLockHolder(const Lockable* lockable) + ABSL_EXCLUSIVE_LOCK_FUNCTION(*lockable) + : lockable_{lockable} { + lockable_->Acquire(); + } + + ~ThreadLockHolder() ABSL_UNLOCK_FUNCTION() { lockable_->Release(); } + + private: + Lockable const* lockable_; +}; + +} // namespace nearby +} // namespace location + +#endif // PLATFORM_PUBLIC_LOCKABLE_H_ diff --git a/cpp/platform/public/multi_thread_executor.h b/cpp/platform/public/multi_thread_executor.h index e5160295..6d7ba093 100644 --- a/cpp/platform/public/multi_thread_executor.h +++ b/cpp/platform/public/multi_thread_executor.h @@ -17,6 +17,7 @@ #include "platform/api/platform.h" #include "platform/public/submittable_executor.h" +#include "absl/base/thread_annotations.h" namespace location { namespace nearby { @@ -25,7 +26,7 @@ namespace nearby { // unbounded queue. // // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Executors.html#newFixedThreadPool-int- -class MultiThreadExecutor final : public SubmittableExecutor { +class ABSL_LOCKABLE MultiThreadExecutor final : public SubmittableExecutor { public: using Platform = api::ImplementationPlatform; explicit MultiThreadExecutor(int max_parallelism) diff --git a/cpp/platform/public/multi_thread_executor_test.cc b/cpp/platform/public/multi_thread_executor_test.cc index ef35acd2..5a405d4e 100644 --- a/cpp/platform/public/multi_thread_executor_test.cc +++ b/cpp/platform/public/multi_thread_executor_test.cc @@ -104,5 +104,35 @@ TEST(MultiThreadExecutorTest, CanSubmit) { EXPECT_TRUE(future.Get().result()); } +struct ThreadCheckTestClass { + MultiThreadExecutor executor{kMaxThreads}; + int value ABSL_GUARDED_BY(executor) = 0; + + void incValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { value++; } + int getValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { return value; } +}; + +TEST(MultiThreadExecutorTest, ThreadCheck_ExecuteRunnable) { + ThreadCheckTestClass test_class; + + test_class.executor.Execute( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }); +} + +TEST(MultiThreadExecutorTest, ThreadCheck_SubmitCallable) { + ThreadCheckTestClass test_class; + Future future; + + bool submitted = test_class.executor.Submit( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + return ExceptionOr{test_class.getValue()}; + }, + &future); + + EXPECT_TRUE(submitted); + EXPECT_EQ(future.Get().result(), 0); +} } // namespace nearby } // namespace location diff --git a/cpp/platform/public/scheduled_executor.h b/cpp/platform/public/scheduled_executor.h index 4ce64867..bd603887 100644 --- a/cpp/platform/public/scheduled_executor.h +++ b/cpp/platform/public/scheduled_executor.h @@ -24,8 +24,12 @@ #include "platform/base/runnable.h" #include "platform/public/cancelable.h" #include "platform/public/cancellable_task.h" +#include "platform/public/lockable.h" #include "platform/public/mutex.h" #include "platform/public/mutex_lock.h" +#include "platform/public/thread_check_callable.h" +#include "platform/public/thread_check_runnable.h" +#include "absl/base/thread_annotations.h" #include "absl/time/time.h" namespace location { @@ -35,7 +39,7 @@ namespace nearby { // execute periodically. // // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ScheduledExecutorService.html -class ScheduledExecutor final { +class ABSL_LOCKABLE ScheduledExecutor final : public Lockable { public: using Platform = api::ImplementationPlatform; @@ -57,7 +61,7 @@ class ScheduledExecutor final { } void Execute(Runnable&& runnable) ABSL_LOCKS_EXCLUDED(mutex_) { MutexLock lock(&mutex_); - if (impl_) impl_->Execute(std::move(runnable)); + if (impl_) impl_->Execute(ThreadCheckRunnable(this, std::move(runnable))); } void Shutdown() ABSL_LOCKS_EXCLUDED(mutex_) { @@ -75,7 +79,8 @@ class ScheduledExecutor final { ABSL_LOCKS_EXCLUDED(mutex_) { MutexLock lock(&mutex_); if (impl_) { - auto task = std::make_shared(std::move(runnable)); + auto task = std::make_shared( + ThreadCheckRunnable(this, std::move(runnable))); return Cancelable(task, impl_->Schedule([task]() { (*task)(); }, duration)); } else { diff --git a/cpp/platform/public/scheduled_executor_test.cc b/cpp/platform/public/scheduled_executor_test.cc index f2469373..96e120ad 100644 --- a/cpp/platform/public/scheduled_executor_test.cc +++ b/cpp/platform/public/scheduled_executor_test.cc @@ -175,5 +175,32 @@ TEST(ScheduledExecutorTest, EXPECT_EQ(value, 1); } +struct ThreadCheckTestClass { + ScheduledExecutor executor; + int value ABSL_GUARDED_BY(executor) = 0; + + void incValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { value++; } + int getValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { return value; } +}; + +TEST(ScheduledExecutorTest, ThreadCheck_Execute) { + ThreadCheckTestClass test_class; + + test_class.executor.Execute( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }); +} + +TEST(ScheduledExecutorTest, ThreadCheck_Schedule) { + ThreadCheckTestClass test_class; + + test_class.executor.Schedule( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }, + absl::ZeroDuration()); +} + } // namespace nearby } // namespace location diff --git a/cpp/platform/public/single_thread_executor.h b/cpp/platform/public/single_thread_executor.h index f832b29f..015e5c58 100644 --- a/cpp/platform/public/single_thread_executor.h +++ b/cpp/platform/public/single_thread_executor.h @@ -16,6 +16,7 @@ #define PLATFORM_PUBLIC_SINGLE_THREAD_EXECUTOR_H_ #include "platform/public/submittable_executor.h" +#include "absl/base/thread_annotations.h" namespace location { namespace nearby { @@ -24,7 +25,7 @@ namespace nearby { // queue. // // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor-- -class SingleThreadExecutor final : public SubmittableExecutor { +class ABSL_LOCKABLE SingleThreadExecutor final : public SubmittableExecutor { public: using Platform = api::ImplementationPlatform; SingleThreadExecutor() diff --git a/cpp/platform/public/single_thread_executor_test.cc b/cpp/platform/public/single_thread_executor_test.cc index 488a529c..f89773f3 100644 --- a/cpp/platform/public/single_thread_executor_test.cc +++ b/cpp/platform/public/single_thread_executor_test.cc @@ -81,5 +81,40 @@ TEST(SingleThreadExecutorTest, CanSubmit) { EXPECT_TRUE(future.Get().result()); } +struct ThreadCheckTestClass { + SingleThreadExecutor executor; + int value ABSL_GUARDED_BY(executor) = 0; + + void incValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { value++; } + int getValue() ABSL_EXCLUSIVE_LOCKS_REQUIRED(executor) { return value; } +}; + +TEST(SingleThreadExecutorTest, ThreadCheck_ExecuteRunnable) { + ThreadCheckTestClass test_class; + + test_class.executor.Execute( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }); +} + +TEST(SingleThreadExecutorTest, ThreadCheck_SubmitCallable) { + ThreadCheckTestClass test_class; + test_class.executor.Execute( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }); + Future future; + + bool submitted = test_class.executor.Submit( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + return ExceptionOr{test_class.getValue()}; + }, + &future); + + EXPECT_TRUE(submitted); + EXPECT_EQ(future.Get().result(), 1); +} + } // namespace nearby } // namespace location diff --git a/cpp/platform/public/submittable_executor.h b/cpp/platform/public/submittable_executor.h index 0e6093e1..c5c6f3ab 100644 --- a/cpp/platform/public/submittable_executor.h +++ b/cpp/platform/public/submittable_executor.h @@ -25,8 +25,12 @@ #include "platform/base/callable.h" #include "platform/base/runnable.h" #include "platform/public/future.h" +#include "platform/public/lockable.h" #include "platform/public/mutex.h" #include "platform/public/mutex_lock.h" +#include "platform/public/thread_check_callable.h" +#include "platform/public/thread_check_runnable.h" +#include "absl/base/thread_annotations.h" namespace location { namespace nearby { @@ -36,7 +40,8 @@ inline int GetCurrentTid() { return api::GetCurrentTid(); } // Main interface to be used by platform as a base class for // - MultiThreadExecutor // - SingleThreadExecutor -class SubmittableExecutor : public api::SubmittableExecutor { +class ABSL_LOCKABLE SubmittableExecutor : public api::SubmittableExecutor, + public Lockable { public: ~SubmittableExecutor() override { MutexLock lock(&mutex_); @@ -54,7 +59,7 @@ class SubmittableExecutor : public api::SubmittableExecutor { } void Execute(Runnable&& runnable) ABSL_LOCKS_EXCLUDED(mutex_) override { MutexLock lock(&mutex_); - if (impl_) impl_->Execute(std::move(runnable)); + if (impl_) impl_->Execute(ThreadCheckRunnable(this, std::move(runnable))); } int GetTid(int index) const ABSL_LOCKS_EXCLUDED(mutex_) override { @@ -74,14 +79,16 @@ class SubmittableExecutor : public api::SubmittableExecutor { bool Submit(Callable&& callable, Future* future) ABSL_LOCKS_EXCLUDED(mutex_) { MutexLock lock(&mutex_); - bool submitted = DoSubmit([callable{std::move(callable)}, future]() { - ExceptionOr result = callable(); - if (result.ok()) { - future->Set(result.result()); - } else { - future->SetException({result.exception()}); - } - }); + bool submitted = + DoSubmit([callable = ThreadCheckCallable(this, std::move(callable)), + future]() { + ExceptionOr result = callable(); + if (result.ok()) { + future->Set(result.result()); + } else { + future->SetException({result.exception()}); + } + }); if (!submitted) { // complete immediately with kExecution exception value. future->SetException({Exception::kExecution}); diff --git a/cpp/platform/public/thread_check_callable.h b/cpp/platform/public/thread_check_callable.h new file mode 100644 index 00000000..cf324b1f --- /dev/null +++ b/cpp/platform/public/thread_check_callable.h @@ -0,0 +1,46 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef PLATFORM_PUBLIC_THREAD_CHECK_CALLABLE_H_ +#define PLATFORM_PUBLIC_THREAD_CHECK_CALLABLE_H_ + +#include "platform/base/callable.h" +#include "platform/public/lockable.h" +#include "absl/base/thread_annotations.h" + +namespace location { +namespace nearby { + +// A callable that acquires a lockable resource while running. +// This class helps with thread safety analysis. +template +class ThreadCheckCallable { + public: + ThreadCheckCallable(const Lockable *lockable, Callable &&callable) + : lockable_{lockable}, callable_{callable} {} + + ExceptionOr operator()() const { + ThreadLockHolder thread_lock(lockable_); + return callable_(); + } + + private: + Lockable const *lockable_; + Callable callable_; +}; + +} // namespace nearby +} // namespace location + +#endif // PLATFORM_PUBLIC_THREAD_CHECK_CALLABLE_H_ diff --git a/cpp/platform/public/thread_check_nocompile.cc b/cpp/platform/public/thread_check_nocompile.cc new file mode 100644 index 00000000..f7cae738 --- /dev/null +++ b/cpp/platform/public/thread_check_nocompile.cc @@ -0,0 +1,83 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "platform/public/scheduled_executor.h" +#include "platform/public/single_thread_executor.h" +#include "absl/time/time.h" + +// Snippets of invalid code that should trigger an error during thread +// safety analysis at compile time. +// See https://g3doc.corp.google.com/googletest/g3doc/cpp_nc_test.md +namespace location { +namespace nearby { + +#ifdef TEST_EXECUTE_MISSING_METHOD_ANNOTATION +struct ThreadCheckTestClass { + SingleThreadExecutor executor; + int value ABSL_GUARDED_BY(executor) = 0; + + void incValue() { value++; } +}; + +void TestExecute_MissingMethodAnnotation() { + ThreadCheckTestClass test_class; + + test_class.executor.Execute( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }); +} +#endif // TEST_EXECUTE_MISSING_METHOD_ANNOTATION + +#ifdef TEST_SUBMIT_MISSING_METHOD_ANNOTATION +struct ThreadCheckTestClass { + SingleThreadExecutor executor; + int value ABSL_GUARDED_BY(executor) = 0; + + int getValue() { return value; } +}; + +void TestSubmit_MissingMethodAnnotation() { + ThreadCheckTestClass test_class; + Future future; + + test_class.executor.Submit( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + return ExceptionOr{test_class.getValue()}; + }, + &future); +} +#endif // TEST_SUBMIT_MISSING_METHOD_ANNOTATION + +#ifdef TEST_SCHEDULE_MISSING_METHOD_ANNOTATION +struct ThreadCheckTestClass { + ScheduledExecutor executor; + int value ABSL_GUARDED_BY(executor) = 0; + + void incValue() { value++; } +}; + +void TestSchedule_MissingMethodAnnotation() { + ThreadCheckTestClass test_class; + + test_class.executor.Schedule( + [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) { + test_class.incValue(); + }, + absl::ZeroDuration()); +} +#endif // TEST_SCHEDULE_MISSING_METHOD_ANNOTATION + +} // namespace nearby +} // namespace location diff --git a/cpp/platform/public/thread_check_nocompile_test.py b/cpp/platform/public/thread_check_nocompile_test.py new file mode 100644 index 00000000..56577960 --- /dev/null +++ b/cpp/platform/public/thread_check_nocompile_test.py @@ -0,0 +1,26 @@ +"""Negative tests for thread safety analysis in executors implementation.""" + +from google3.testing.pybase import fake_target_util +from google3.testing.pybase import googletest + + +class ThreadCheckNocompileTest(googletest.TestCase): + """Negative tests for thread safety analysis in executors implementation.""" + + def testCompilerErrors(self): + """Runs a list of tests to verify that erroneous code leads to expected compiler messages.""" + test_specs = [ + ('EXECUTE_MISSING_METHOD_ANNOTATION', [r'-Wthread-safety-analysis']), + ('SUBMIT_MISSING_METHOD_ANNOTATION', [r'-Wthread-safety-analysis']), + ('SCHEDULE_MISSING_METHOD_ANNOTATION', [r'-Wthread-safety-analysis']), + # Tests that compiling a valid C++ succeeds. + ('SANITY', None) # None means that the compilation should succeed. + ] + fake_target_util.AssertCcCompilerErrors( + self, + 'google3/platform/public/thread_check_nocompile', + 'thread_check_nocompile.o', test_specs) + + +if __name__ == '__main__': + googletest.main() diff --git a/cpp/platform/public/thread_check_runnable.h b/cpp/platform/public/thread_check_runnable.h new file mode 100644 index 00000000..7abf6f78 --- /dev/null +++ b/cpp/platform/public/thread_check_runnable.h @@ -0,0 +1,47 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef PLATFORM_PUBLIC_THREAD_CHECK_RUNNABLE_H_ +#define PLATFORM_PUBLIC_THREAD_CHECK_RUNNABLE_H_ + +#include + +#include "platform/base/runnable.h" +#include "platform/public/lockable.h" +#include "absl/base/thread_annotations.h" + +namespace location { +namespace nearby { + +// A runnable that acquires a lockable resource while running. +// This class helps with thread safety analysis. +class ThreadCheckRunnable { + public: + ThreadCheckRunnable(const Lockable *lockable, Runnable &&runnable) + : lockable_{lockable}, runnable_{runnable} {} + + void operator()() const { + ThreadLockHolder thread_lock(lockable_); + runnable_(); + } + + private: + Lockable const *lockable_; + Runnable runnable_; +}; + +} // namespace nearby +} // namespace location + +#endif // PLATFORM_PUBLIC_THREAD_CHECK_RUNNABLE_H_