Shatter JvmRefBase and ThreadGuard into separate files.

implementation/BUILD

@@ -546,6 +546,18 @@ cc_library(
     ],
 )
 
+cc_test(
+    name = "jvm_test",
+    srcs = ["jvm_test.cc"],
+    deps = [
+        "//:jni_bind",
+        "//:jni_test",
+        "//implementation/jni_helper",
+        "//implementation/jni_helper:jni_env",
+        "@googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "jvm_ref",
     hdrs = ["jvm_ref.h"],
@@ -559,8 +571,10 @@ cc_library(
         ":global_class_loader",
         ":jni_type",
         ":jvm",
+        ":jvm_ref_base",
         ":promotion_mechanics_tags",
         ":ref_storage",
+        ":thread_guard",
         "//:jni_dep",
         "//class_defs:java_lang_classes",
         "//implementation/jni_helper",
@@ -570,14 +584,22 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "jvm_ref_base",
+    hdrs = ["jvm_ref_base.h"],
+    deps = [
+        ":forward_declarations",
+        "//:jni_dep",
+    ],
+)
+
 cc_test(
-    name = "jvm_test",
-    srcs = ["jvm_test.cc"],
+    name = "jvm_ref_test",
+    srcs = ["jvm_ref_test.cc"],
     deps = [
+        ":jvm_ref",
         "//:jni_bind",
         "//:jni_test",
-        "//implementation/jni_helper",
-        "//implementation/jni_helper:jni_env",
         "@googletest//:gtest_main",
     ],
 )
@@ -1190,6 +1212,28 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "thread_guard",
+    hdrs = ["thread_guard.h"],
+    deps = [
+        ":forward_declarations",
+        ":jvm_ref_base",
+        "//:jni_dep",
+        "//metaprogramming:function_traits",
+    ],
+)
+
+cc_test(
+    name = "thread_guard_test",
+    srcs = ["thread_guard_test.cc"],
+    deps = [
+        ":thread_guard",
+        "//:jni_bind",
+        "//:jni_test",
+        "@googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "void",
     hdrs = ["void.h"],

implementation/forward_declarations.h

@@ -65,6 +65,10 @@ template <LifecycleType lifecycleType, const auto& jvm_v_,
           const auto& class_loader_v_>
 class ClassLoaderRef;
 
+// Jvm.
+template <const auto& jvm_v_>
+class JvmRef;
+
 // Thread Guards.
 class ThreadGuard;
 class ThreadLocalGuardDestructor;

implementation/jvm_ref.h

@@ -33,117 +33,16 @@
 #include "implementation/jni_helper/lifecycle_object.h"
 #include "implementation/jni_type.h"
 #include "implementation/jvm.h"
+#include "implementation/jvm_ref_base.h"
 #include "implementation/promotion_mechanics_tags.h"
 #include "implementation/ref_storage.h"
+#include "implementation/thread_guard.h"
 #include "jni_dep.h"
 #include "metaprogramming/double_locked_value.h"
 #include "metaprogramming/function_traits.h"
 
 namespace jni {
 
-template <const auto& jvm_v_ = kDefaultJvm>
-class JvmRef;
-
-// Helper for JvmRef to enforce correct sequencing of getting and setting
-// process level static fo JavaVM*.
-class JvmRefBase {
- protected:
-  friend class ThreadGuard;
-  friend class ThreadLocalGuardDestructor;
-
-  JvmRefBase(JavaVM* vm) { process_level_jvm_.store(vm); }
-  ~JvmRefBase() { process_level_jvm_.store(nullptr); }
-
-  static JavaVM* GetJavaVm() { return process_level_jvm_.load(); }
-  static void SetJavaVm(JavaVM* jvm) { process_level_jvm_.store(jvm); }
-
-  static inline std::atomic<JavaVM*> process_level_jvm_ = nullptr;
-};
-
-// Designed to be the very last JniBind object to execute on the thread.
-// Objects passed by move for lambdas will be destructed after any contents
-// statements within their lambda, and `ThreadGuard` can't be moved into the
-// lambda because its construction will be on the host thread. This static
-// teardown guarantees a delayed destruction beyond any GlobalObject.
-class ThreadLocalGuardDestructor {
- public:
-  bool detach_thread_when_all_guards_released_ = false;
-
-  // By calling this the compiler is obligated to perform initalisation.
-  void ForceDestructionOnThreadClose() {}
-
-  ~ThreadLocalGuardDestructor() {
-    if (detach_thread_when_all_guards_released_) {
-      JavaVM* jvm = JvmRefBase::GetJavaVm();
-      if (jvm) {
-        jvm->DetachCurrentThread();
-      }
-    }
-  }
-};
-
-// ThreadGuard attaches and detaches JNIEnv* objects on the creation of new
-// threads.  All new threads which want to use JNI Wrapper must hold a
-// ThreadGuard beyond the scope of all created objects.  If the ThreadGuard
-// needs to create an Env, it will also detach itself.
-class ThreadGuard {
- public:
-  ~ThreadGuard() {
-    thread_guard_count_--;
-  }
-
-  ThreadGuard(ThreadGuard&) = delete;
-  ThreadGuard(ThreadGuard&&) = delete;
-
-  template <const auto& jvm_v_>
-  friend class JvmRef;
-
-  // This constructor must *never* be called before a |JvmRef| has been
-  // constructed. It depends on static setup from |JvmRef|.
-  [[nodiscard]] ThreadGuard() {
-    thread_local_guard_destructor.ForceDestructionOnThreadClose();
-
-    // Nested ThreadGuards should be permitted in the same way mutex locks are.
-    thread_guard_count_++;
-    if (thread_guard_count_ != 1) {
-      // SetEnv has been called prior, GetEnv is currently valid.
-      return;
-    }
-
-    // Declarations for AttachCurrentThread are inconsistent across different
-    // JNI headers.  This forces a cast to whatever the expected type is.
-    JavaVM* const vm = JvmRefBase::GetJavaVm();
-    JNIEnv* jni_env = 0;
-
-    using TypeForGetEnv =
-        metaprogramming::FunctionTraitsArg_t<decltype(&JavaVM::GetEnv), 1>;
-    const int code =
-        vm->GetEnv(reinterpret_cast<TypeForGetEnv>(&jni_env), JNI_VERSION_1_6);
-
-    if (code != JNI_OK) {
-      using TypeForAttachment = metaprogramming::FunctionTraitsArg_t<
-          decltype(&JavaVM::AttachCurrentThread), 1>;
-      vm->AttachCurrentThread(reinterpret_cast<TypeForAttachment>(&jni_env),
-                              nullptr);
-      thread_local_guard_destructor.detach_thread_when_all_guards_released_ =
-          true;
-    }
-    // Why not store this locally to ThreadGuard?
-    //
-    // JNIEnv is thread local static, and the context an object is built from
-    // may not have easy access to a JNIEnv* (or this ThreadGuard).  For most
-    // constructions of new objects, the env is likely trivial (it's passed as
-    // part of the JNI call), however, if an object reference is moved from one
-    // thread to another, the JNIEnv* is certainly not available.
-    JniEnv::SetEnv(jni_env);
-  }
-
- private:
-  static inline thread_local int thread_guard_count_ = 0;
-  static inline thread_local ThreadLocalGuardDestructor
-      thread_local_guard_destructor{};
-};
-
 // Represents a runtime instance of a Java Virtual Machine.
 // The caller is responsible for dropping this object from scope when
 // JNI_OnUnload is called.

implementation/jvm_ref_base.h

@@ -0,0 +1,45 @@
+/*
+ * Copyright 2024 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
+ *
+ *      http://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 JNI_BIND_IMPLEMENTATION_JVM_REF_BASE_H_
+#define JNI_BIND_IMPLEMENTATION_JVM_REF_BASE_H_
+
+#include <atomic>
+
+#include "implementation/forward_declarations.h"
+#include "jni_dep.h"
+
+namespace jni {
+
+// Helper for JvmRef to enforce correct sequencing of getting and setting
+// process level static fo JavaVM*.
+class JvmRefBase {
+ protected:
+  friend class ThreadGuard;
+  friend class ThreadLocalGuardDestructor;
+
+  JvmRefBase(JavaVM* vm) { process_level_jvm_.store(vm); }
+  ~JvmRefBase() { process_level_jvm_.store(nullptr); }
+
+  static JavaVM* GetJavaVm() { return process_level_jvm_.load(); }
+  static void SetJavaVm(JavaVM* jvm) { process_level_jvm_.store(jvm); }
+
+  static inline std::atomic<JavaVM*> process_level_jvm_ = nullptr;
+};
+
+}  // namespace jni
+
+#endif  // JNI_BIND_IMPLEMENTATION_JVM_REF_BASE_H_

implementation/jvm_ref_test.cc

@@ -0,0 +1,115 @@
+/*
+ * Copyright 2024 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
+ *
+ *      http://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 "implementation/jvm_ref.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include "jni_bind.h"
+#include "jni_test.h"
+
+using ::jni::Class;
+using ::jni::JvmRef;
+using ::jni::LocalObject;
+using ::jni::test::AsGlobal;
+using ::jni::test::Fake;
+using ::jni::test::JniTest;
+using ::jni::test::JniTestWithNoDefaultJvmRef;
+using ::testing::AnyNumber;
+using ::testing::Return;
+
+namespace {
+
+TEST_F(JniTest, JvmRefTearsDownClassesLoadedfromDefaultLoader) {
+  // Note, this expectation means FindClass is called *exactly* once.
+  // Using offset to avoid Fake<jclass>() usage in jni_test.h.
+  EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake<jclass>(1)));
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber());
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(1))));
+
+  static constexpr Class kClass1{"com/google/Class1"};
+  LocalObject<kClass1> local_object1{};
+}
+
+TEST_F(JniTest, NoStaticCrossTalkWithPriorTest) {
+  // Note, this expectation means FindClass is called *exactly* once.
+  // Using 1 offset to avoid Fake<jclass>() usage in jni_test.h.
+  EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake<jclass>(1)));
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber());
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(1))));
+
+  static constexpr Class kClass1{"com/google/Class1"};
+  LocalObject<kClass1> local_object1{};
+}
+
+TEST_F(JniTest, NoStaticCrossTalkWithUnrelatedTest) {
+  // Note, this expectation means FindClass is called *exactly* once.
+  // Using 1 offset to avoid Fake<jclass>() usage in jni_test.h.
+  EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake<jclass>(1)));
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber());
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(1))));
+
+  static constexpr Class kClass1{"com/google/Class2"};
+  LocalObject<kClass1> local_object1{};
+}
+
+TEST_F(JniTestWithNoDefaultJvmRef,
+       JvmsNeitherQueryNorReleaseIfNoObjectsCreated) {
+  JvmRef<jni::kDefaultJvm> jvm_ref(jvm_.get());
+  EXPECT_CALL(*env_, FindClass).Times(0);
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(0);
+}
+
+TEST_F(JniTestWithNoDefaultJvmRef, JvmRefsDontReuuseStaleFindClassValues) {
+  EXPECT_CALL(*env_, FindClass)
+      .WillOnce(Return(Fake<jclass>(1)))
+      .WillOnce(Return(Fake<jclass>(2)));
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber());
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(1))));
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(2))));
+
+  static constexpr Class kClass1{"com/google/ADifferentClassForVariety"};
+  {
+    JvmRef<jni::kDefaultJvm> jvm_ref(jvm_.get());
+    LocalObject<kClass1> local_object1{};
+  }
+
+  {
+    JvmRef<jni::kDefaultJvm> jvm_ref(jvm_.get());
+    LocalObject<kClass1> local_object1{};
+  }
+}
+
+TEST_F(JniTest, DefaultLoaderReleasesMultipleClasses) {
+  EXPECT_CALL(*env_, FindClass)
+      .WillOnce(Return(Fake<jclass>(1)))
+      .WillOnce(Return(Fake<jclass>(2)))
+      .WillOnce(Return(Fake<jclass>(3)));
+  EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber());
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(1))));
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(2))));
+  EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake<jclass>(3))));
+
+  static constexpr Class kClass1{"com/google/Class1"};
+  static constexpr Class kClass2{"com/google/Class2"};
+  static constexpr Class kClass3{"com/google/Class3"};
+
+  LocalObject<kClass1> local_object1{};
+  LocalObject<kClass2> local_object2{};
+  LocalObject<kClass3> local_object3{};
+}
+
+}  // namespace