Internal change

PiperOrigin-RevId: 361877303

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",
+    ],
+)

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_

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)

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<int> future;
+
+  bool submitted = test_class.executor.Submit<int>(
+      [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) {
+        return ExceptionOr<int>{test_class.getValue()};
+      },
+      &future);
+
+  EXPECT_TRUE(submitted);
+  EXPECT_EQ(future.Get().result(), 0);
+}
 }  // namespace nearby
 }  // namespace location

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<CancellableTask>(std::move(runnable));
+      auto task = std::make_shared<CancellableTask>(
+          ThreadCheckRunnable(this, std::move(runnable)));
       return Cancelable(task,
                         impl_->Schedule([task]() { (*task)(); }, duration));
     } else {

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

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()

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<int> future;
+
+  bool submitted = test_class.executor.Submit<int>(
+      [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) {
+        return ExceptionOr<int>{test_class.getValue()};
+      },
+      &future);
+
+  EXPECT_TRUE(submitted);
+  EXPECT_EQ(future.Get().result(), 1);
+}
+
 }  // namespace nearby
 }  // namespace location

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<T>&& callable, Future<T>* future)
       ABSL_LOCKS_EXCLUDED(mutex_) {
     MutexLock lock(&mutex_);
-    bool submitted = DoSubmit([callable{std::move(callable)}, future]() {
-      ExceptionOr<T> result = callable();
-      if (result.ok()) {
-        future->Set(result.result());
-      } else {
-        future->SetException({result.exception()});
-      }
-    });
+    bool submitted =
+        DoSubmit([callable = ThreadCheckCallable<T>(this, std::move(callable)),
+                  future]() {
+          ExceptionOr<T> 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});

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 <typename T>
+class ThreadCheckCallable {
+ public:
+  ThreadCheckCallable(const Lockable *lockable, Callable<T> &&callable)
+      : lockable_{lockable}, callable_{callable} {}
+
+  ExceptionOr<T> operator()() const {
+    ThreadLockHolder thread_lock(lockable_);
+    return callable_();
+  }
+
+ private:
+  Lockable const *lockable_;
+  Callable<T> callable_;
+};
+
+}  // namespace nearby
+}  // namespace location
+
+#endif  // PLATFORM_PUBLIC_THREAD_CHECK_CALLABLE_H_

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<int> future;
+
+  test_class.executor.Submit<int>(
+      [&test_class]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(test_class.executor) {
+        return ExceptionOr<int>{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

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()

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 <utility>
+
+#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_