From 292d8b2c91bc0fefc21ef952c4a7322360848e5b Mon Sep 17 00:00:00 2001 From: Jamieson Pryor Date: Mon, 8 Jan 2024 11:19:02 -0800 Subject: [PATCH] Shatter `JvmRefBase` and `ThreadGuard` into separate files. This change has no effect. PiperOrigin-RevId: 596651494 --- implementation/BUILD | 52 +++++- implementation/forward_declarations.h | 4 + implementation/jvm_ref.h | 105 +---------- implementation/jvm_ref_base.h | 45 +++++ implementation/jvm_ref_test.cc | 115 ++++++++++++ implementation/jvm_test.cc | 241 -------------------------- implementation/thread_guard.h | 111 ++++++++++++ implementation/thread_guard_test.cc | 205 ++++++++++++++++++++++ 8 files changed, 530 insertions(+), 348 deletions(-) create mode 100644 implementation/jvm_ref_base.h create mode 100644 implementation/jvm_ref_test.cc create mode 100644 implementation/thread_guard.h create mode 100644 implementation/thread_guard_test.cc diff --git a/implementation/BUILD b/implementation/BUILD index 5d11fdf0..e32fdcd2 100644 --- a/implementation/BUILD +++ b/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"], diff --git a/implementation/forward_declarations.h b/implementation/forward_declarations.h index 581307d5..3647abb6 100644 --- a/implementation/forward_declarations.h +++ b/implementation/forward_declarations.h @@ -65,6 +65,10 @@ template class ClassLoaderRef; +// Jvm. +template +class JvmRef; + // Thread Guards. class ThreadGuard; class ThreadLocalGuardDestructor; diff --git a/implementation/jvm_ref.h b/implementation/jvm_ref.h index 17ad434b..e2fd2de4 100644 --- a/implementation/jvm_ref.h +++ b/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 -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 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 - 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; - const int code = - vm->GetEnv(reinterpret_cast(&jni_env), JNI_VERSION_1_6); - - if (code != JNI_OK) { - using TypeForAttachment = metaprogramming::FunctionTraitsArg_t< - decltype(&JavaVM::AttachCurrentThread), 1>; - vm->AttachCurrentThread(reinterpret_cast(&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. diff --git a/implementation/jvm_ref_base.h b/implementation/jvm_ref_base.h new file mode 100644 index 00000000..ab154758 --- /dev/null +++ b/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 + +#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 process_level_jvm_ = nullptr; +}; + +} // namespace jni + +#endif // JNI_BIND_IMPLEMENTATION_JVM_REF_BASE_H_ diff --git a/implementation/jvm_ref_test.cc b/implementation/jvm_ref_test.cc new file mode 100644 index 00000000..e22ac476 --- /dev/null +++ b/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 +#include +#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() usage in jni_test.h. + EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); + EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); + + static constexpr Class kClass1{"com/google/Class1"}; + LocalObject local_object1{}; +} + +TEST_F(JniTest, NoStaticCrossTalkWithPriorTest) { + // Note, this expectation means FindClass is called *exactly* once. + // Using 1 offset to avoid Fake() usage in jni_test.h. + EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); + EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); + + static constexpr Class kClass1{"com/google/Class1"}; + LocalObject local_object1{}; +} + +TEST_F(JniTest, NoStaticCrossTalkWithUnrelatedTest) { + // Note, this expectation means FindClass is called *exactly* once. + // Using 1 offset to avoid Fake() usage in jni_test.h. + EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); + EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); + + static constexpr Class kClass1{"com/google/Class2"}; + LocalObject local_object1{}; +} + +TEST_F(JniTestWithNoDefaultJvmRef, + JvmsNeitherQueryNorReleaseIfNoObjectsCreated) { + JvmRef 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(1))) + .WillOnce(Return(Fake(2))); + EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(2)))); + + static constexpr Class kClass1{"com/google/ADifferentClassForVariety"}; + { + JvmRef jvm_ref(jvm_.get()); + LocalObject local_object1{}; + } + + { + JvmRef jvm_ref(jvm_.get()); + LocalObject local_object1{}; + } +} + +TEST_F(JniTest, DefaultLoaderReleasesMultipleClasses) { + EXPECT_CALL(*env_, FindClass) + .WillOnce(Return(Fake(1))) + .WillOnce(Return(Fake(2))) + .WillOnce(Return(Fake(3))); + EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(2)))); + EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(3)))); + + static constexpr Class kClass1{"com/google/Class1"}; + static constexpr Class kClass2{"com/google/Class2"}; + static constexpr Class kClass3{"com/google/Class3"}; + + LocalObject local_object1{}; + LocalObject local_object2{}; + LocalObject local_object3{}; +} + +} // namespace diff --git a/implementation/jvm_test.cc b/implementation/jvm_test.cc index fcb85ec8..5c7cfb7d 100644 --- a/implementation/jvm_test.cc +++ b/implementation/jvm_test.cc @@ -72,245 +72,4 @@ TEST(Jni, JvmStaticClassLoaderLookupsWork) { static_assert(kJvm3.IdxOfClassLoader() == 2); } -TEST_F(JniTest, JvmRefTearsDownClassesLoadedfromDefaultLoader) { - // Note, this expectation means FindClass is called *exactly* once. - // Using offset to avoid Fake() usage in jni_test.h. - EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); - EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); - - static constexpr Class kClass1{"com/google/Class1"}; - LocalObject local_object1{}; -} - -TEST_F(JniTest, NoStaticCrossTalkWithPriorTest) { - // Note, this expectation means FindClass is called *exactly* once. - // Using 1 offset to avoid Fake() usage in jni_test.h. - EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); - EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); - - static constexpr Class kClass1{"com/google/Class1"}; - LocalObject local_object1{}; -} - -TEST_F(JniTest, NoStaticCrossTalkWithUnrelatedTest) { - // Note, this expectation means FindClass is called *exactly* once. - // Using 1 offset to avoid Fake() usage in jni_test.h. - EXPECT_CALL(*env_, FindClass).WillOnce(Return(Fake(1))); - EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); - - static constexpr Class kClass1{"com/google/Class2"}; - LocalObject local_object1{}; -} - -TEST_F(JniTestWithNoDefaultJvmRef, - JvmsNeitherQueryNorReleaseIfNoObjectsCreated) { - JvmRef 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(1))) - .WillOnce(Return(Fake(2))); - EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(2)))); - - static constexpr Class kClass1{"com/google/ADifferentClassForVariety"}; - { - JvmRef jvm_ref(jvm_.get()); - LocalObject local_object1{}; - } - - { - JvmRef jvm_ref(jvm_.get()); - LocalObject local_object1{}; - } -} - -TEST_F(JniTest, DefaultLoaderReleasesMultipleClasses) { - EXPECT_CALL(*env_, FindClass) - .WillOnce(Return(Fake(1))) - .WillOnce(Return(Fake(2))) - .WillOnce(Return(Fake(3))); - EXPECT_CALL(*env_, DeleteGlobalRef).Times(AnyNumber()); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(1)))); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(2)))); - EXPECT_CALL(*env_, DeleteGlobalRef(AsGlobal(Fake(3)))); - - static constexpr Class kClass1{"com/google/Class1"}; - static constexpr Class kClass2{"com/google/Class2"}; - static constexpr Class kClass3{"com/google/Class3"}; - - LocalObject local_object1{}; - LocalObject local_object2{}; - LocalObject local_object3{}; -} - -//////////////////////////////////////////////////////////////////////////////// -// ThreadGuard Tests. -//////////////////////////////////////////////////////////////////////////////// - -TEST(ThreadGuard, - NeverCallsAttachOrDetachCurrentThreadIfAnEnvIsAlreadyAttached) { - MockJvm jvm; - EXPECT_CALL(jvm, GetEnv).WillOnce(Return(JNI_OK)); - EXPECT_CALL(jvm, AttachCurrentThread).Times(0); - EXPECT_CALL(jvm, DetachCurrentThread).Times(0); - - JvmRef jvm_ref{&jvm}; -} - -TEST(ThreadGuard, WontEffectDetachmentForPreexistingEnv) { - MockJvm jvm; - EXPECT_CALL(jvm, GetEnv).WillOnce(Return(JNI_OK)); - EXPECT_CALL(jvm, AttachCurrentThread).Times(0); - EXPECT_CALL(jvm, DetachCurrentThread).Times(0); - - JvmRef jvm_ref{&jvm}; - ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); -} - -// This behaviour is no longer tested! To guarantee GlobalObjects passed into -// lambdas are supported, this happens on thread teardown, which is past -// a point when it can be tested on main thread in unit testing. -/* -TEST(ThreadGuard, CallsAttachCurrentThreadIfEnvIsNotAttached) { - MockJvm jvm; - EXPECT_CALL(jvm, GetEnv).WillRepeatedly(Return(JNI_EDETACHED)); - EXPECT_CALL(jvm, AttachCurrentThread).Times(1); - EXPECT_CALL(jvm, DetachCurrentThread).Times(1); - - JvmRef jvm_ref{&jvm}; -} - -TEST(JvmThreadGuard, DetachesOnceForTheMainJvmRefAndThreadGuard) { - jni::test::MockJvm jvm; - - EXPECT_CALL(jvm, GetEnv).WillRepeatedly(Return(JNI_EDETACHED)); - EXPECT_CALL(jvm, AttachCurrentThread(_, _)).Times(1); - EXPECT_CALL(jvm, DetachCurrentThread).Times(1); - - // Will call AttachCurrentThread once for the main thread (JvmRef). - // Subsequent thread guards have no effect. - JvmRef jvm_ref{&jvm}; - ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); -} -*/ - -constexpr jni::Class kObjectTestHelperClass{ - "com/jnibind/test/ObjectTestHelper", - - ::jni::Field{"intVal1", int{}}, -}; - -TEST_F(JniTest, AllowsMoveCtorIntoLambdaWithThreadGuardUsage) { - GlobalObject global_obj{PromoteToGlobal{}, - Fake(1)}; - - std::thread worker{[gobj{std::move(global_obj)}]() mutable { - ThreadGuard thread_guard{}; - gobj["intVal1"].Get(); - }}; - - worker.join(); -} - -TEST(JvmThreadGuard, DetachesOnceForMultipleGuardsOnSingleThread) { - jni::test::MockJvm jvm; - - EXPECT_CALL(jvm, GetEnv(_, _)).WillRepeatedly(Return(JNI_EDETACHED)); - EXPECT_CALL(jvm, AttachCurrentThread(_, _)).Times(1); - - // See above uncommented test. - // EXPECT_CALL(jvm, DetachCurrentThread).Times(1); - - // Will call AttachCurrentThread once for the main thread (JvmRef) and once - // for the constructed ThreadGuard. - JvmRef jvm_ref{&jvm}; - ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_4 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_5 = jvm_ref.BuildThreadGuard(); -} - -TEST(JvmThreadGuard, UpdatesIndividualThreadsWithNewValues) { - jni::test::MockJvm jvm; - - // This sequence of expectation mimics a normal application. - // A main thread usually exists with a JNIEnv that is attached on your - // behalf, and subsequent thread spawns require an explicit attach/detach. - EXPECT_CALL(jvm, GetEnv(_, _)) - .WillOnce(testing::Invoke([&](void** out_env, int) { - *out_env = reinterpret_cast(0xAAAAAAAAAAA); - return JNI_OK; - })) - .WillRepeatedly(Return(JNI_EDETACHED)); - EXPECT_CALL(jvm, AttachCurrentThread(_, _)) - .WillOnce(testing::Invoke([&](void** out_env, void*) { - *out_env = reinterpret_cast(0xBBBBBBBBBBB); - return JNI_OK; - })) - .WillOnce(testing::Invoke([&](void** out_env, void*) { - *out_env = reinterpret_cast(0xCCCCCCCCCCC); - return JNI_OK; - })); - EXPECT_CALL(jvm, DetachCurrentThread()).Times(2); - - JvmRef jvm_ref{&jvm}; - - std::mutex mutex; - std::vector observed_envs; - - // Each thread will have a JNIEnv that is thread local and unset until the - // ThreadGuard is present and sets it. - std::thread test_thread_1([&]() { - EXPECT_EQ(jni::JniEnv::GetEnv(), nullptr); - ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); - - std::lock_guard lock{mutex}; - observed_envs.push_back(jni::JniEnv::GetEnv()); - }); - std::thread test_thread_2([&]() { - // Note, multiple thread guards are permitted, but they have no effect. - EXPECT_EQ(jni::JniEnv::GetEnv(), nullptr); - ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); - ThreadGuard thread_guard_4 = jvm_ref.BuildThreadGuard(); - - std::lock_guard lock{mutex}; - observed_envs.push_back(jni::JniEnv::GetEnv()); - }); - - test_thread_1.join(); - test_thread_2.join(); - - // Placing EXPECT_EQ calls within the thread lambdas caused unexpected data - // races. Given the first thread is joined and then the second, it would seem - // impossible for this to happen, however, EXPECT_EQ doesn't make guarantees - // about the ordering of expectations (whereas join does). The write guard is - // simply to ensure writes are seen on the main thread (this is all but - // guaranteed to happen anyways, but this is technically more friendly to the - // compiler). - // - // Also, detachment is in TLS destruction, which isn't deterministic, however, - // what matters is that both complete, not really the order. - const std::vector expected_output_1{ - reinterpret_cast(0xBBBBBBBBBBB), - reinterpret_cast(0xCCCCCCCCCCC)}; - const std::vector expected_output_2{ - reinterpret_cast(0xCCCCCCCCCCC), - reinterpret_cast(0xBBBBBBBBBBB)}; - EXPECT_TRUE((observed_envs == expected_output_1 || - observed_envs == expected_output_2)); -} - } // namespace diff --git a/implementation/thread_guard.h b/implementation/thread_guard.h new file mode 100644 index 00000000..0e181686 --- /dev/null +++ b/implementation/thread_guard.h @@ -0,0 +1,111 @@ +/* + * 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_THREAD_GUARD_H_ +#define JNI_BIND_IMPLEMENTATION_THREAD_GUARD_H_ + +#include "implementation/forward_declarations.h" +#include "implementation/jvm_ref_base.h" +#include "jni_dep.h" +#include "metaprogramming/function_traits.h" + +namespace jni { + +// 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 + 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; + const int code = + vm->GetEnv(reinterpret_cast(&jni_env), JNI_VERSION_1_6); + + if (code != JNI_OK) { + using TypeForAttachment = metaprogramming::FunctionTraitsArg_t< + decltype(&JavaVM::AttachCurrentThread), 1>; + vm->AttachCurrentThread(reinterpret_cast(&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{}; +}; + +} // namespace jni + +#endif // JNI_BIND_IMPLEMENTATION_THREAD_GUARD_H_ diff --git a/implementation/thread_guard_test.cc b/implementation/thread_guard_test.cc new file mode 100644 index 00000000..5ca5deea --- /dev/null +++ b/implementation/thread_guard_test.cc @@ -0,0 +1,205 @@ +/* + * 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/thread_guard.h" + +#include + +#include +#include +#include "jni_bind.h" +#include "jni_test.h" + +using ::jni::Class; +using ::jni::ClassLoader; +using ::jni::GlobalObject; +using ::jni::Jvm; +using ::jni::JvmRef; +using ::jni::kNullClassLoader; +using ::jni::LocalObject; +using ::jni::PromoteToGlobal; +using ::jni::SupportedClassSet; +using ::jni::ThreadGuard; +using ::jni::test::AsGlobal; +using ::jni::test::Fake; +using ::jni::test::JniTest; +using ::jni::test::JniTestWithNoDefaultJvmRef; +using ::jni::test::MockJvm; +using ::testing::_; +using ::testing::AnyNumber; +using ::testing::Return; + +namespace { + +TEST(ThreadGuard, + NeverCallsAttachOrDetachCurrentThreadIfAnEnvIsAlreadyAttached) { + MockJvm jvm; + EXPECT_CALL(jvm, GetEnv).WillOnce(Return(JNI_OK)); + EXPECT_CALL(jvm, AttachCurrentThread).Times(0); + EXPECT_CALL(jvm, DetachCurrentThread).Times(0); + + JvmRef jvm_ref{&jvm}; +} + +TEST(ThreadGuard, WontEffectDetachmentForPreexistingEnv) { + MockJvm jvm; + EXPECT_CALL(jvm, GetEnv).WillOnce(Return(JNI_OK)); + EXPECT_CALL(jvm, AttachCurrentThread).Times(0); + EXPECT_CALL(jvm, DetachCurrentThread).Times(0); + + JvmRef jvm_ref{&jvm}; + ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); +} + +// This behaviour is no longer tested! To guarantee GlobalObjects passed into +// lambdas are supported, this happens on thread teardown, which is past +// a point when it can be tested on main thread in unit testing. +/* +TEST(ThreadGuard, CallsAttachCurrentThreadIfEnvIsNotAttached) { + MockJvm jvm; + EXPECT_CALL(jvm, GetEnv).WillRepeatedly(Return(JNI_EDETACHED)); + EXPECT_CALL(jvm, AttachCurrentThread).Times(1); + EXPECT_CALL(jvm, DetachCurrentThread).Times(1); + + JvmRef jvm_ref{&jvm}; +} + +TEST(JvmThreadGuard, DetachesOnceForTheMainJvmRefAndThreadGuard) { + jni::test::MockJvm jvm; + + EXPECT_CALL(jvm, GetEnv).WillRepeatedly(Return(JNI_EDETACHED)); + EXPECT_CALL(jvm, AttachCurrentThread(_, _)).Times(1); + EXPECT_CALL(jvm, DetachCurrentThread).Times(1); + + // Will call AttachCurrentThread once for the main thread (JvmRef). + // Subsequent thread guards have no effect. + JvmRef jvm_ref{&jvm}; + ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); +} +*/ + +constexpr jni::Class kObjectTestHelperClass{ + "com/jnibind/test/ObjectTestHelper", + + ::jni::Field{"intVal1", int{}}, +}; + +TEST_F(JniTest, AllowsMoveCtorIntoLambdaWithThreadGuardUsage) { + GlobalObject global_obj{PromoteToGlobal{}, + Fake(1)}; + + std::thread worker{[gobj{std::move(global_obj)}]() mutable { + ThreadGuard thread_guard{}; + gobj["intVal1"].Get(); + }}; + + worker.join(); +} + +TEST(JvmThreadGuard, DetachesOnceForMultipleGuardsOnSingleThread) { + jni::test::MockJvm jvm; + + EXPECT_CALL(jvm, GetEnv(_, _)).WillRepeatedly(Return(JNI_EDETACHED)); + EXPECT_CALL(jvm, AttachCurrentThread(_, _)).Times(1); + + // See above uncommented test. + // EXPECT_CALL(jvm, DetachCurrentThread).Times(1); + + // Will call AttachCurrentThread once for the main thread (JvmRef) and once + // for the constructed ThreadGuard. + JvmRef jvm_ref{&jvm}; + ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_4 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_5 = jvm_ref.BuildThreadGuard(); +} + +TEST(JvmThreadGuard, UpdatesIndividualThreadsWithNewValues) { + jni::test::MockJvm jvm; + + // This sequence of expectation mimics a normal application. + // A main thread usually exists with a JNIEnv that is attached on your + // behalf, and subsequent thread spawns require an explicit attach/detach. + EXPECT_CALL(jvm, GetEnv(_, _)) + .WillOnce(testing::Invoke([&](void** out_env, int) { + *out_env = reinterpret_cast(0xAAAAAAAAAAA); + return JNI_OK; + })) + .WillRepeatedly(Return(JNI_EDETACHED)); + EXPECT_CALL(jvm, AttachCurrentThread(_, _)) + .WillOnce(testing::Invoke([&](void** out_env, void*) { + *out_env = reinterpret_cast(0xBBBBBBBBBBB); + return JNI_OK; + })) + .WillOnce(testing::Invoke([&](void** out_env, void*) { + *out_env = reinterpret_cast(0xCCCCCCCCCCC); + return JNI_OK; + })); + EXPECT_CALL(jvm, DetachCurrentThread()).Times(2); + + JvmRef jvm_ref{&jvm}; + + std::mutex mutex; + std::vector observed_envs; + + // Each thread will have a JNIEnv that is thread local and unset until the + // ThreadGuard is present and sets it. + std::thread test_thread_1([&]() { + EXPECT_EQ(jni::JniEnv::GetEnv(), nullptr); + ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); + + std::lock_guard lock{mutex}; + observed_envs.push_back(jni::JniEnv::GetEnv()); + }); + std::thread test_thread_2([&]() { + // Note, multiple thread guards are permitted, but they have no effect. + EXPECT_EQ(jni::JniEnv::GetEnv(), nullptr); + ThreadGuard thread_guard_1 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_2 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_3 = jvm_ref.BuildThreadGuard(); + ThreadGuard thread_guard_4 = jvm_ref.BuildThreadGuard(); + + std::lock_guard lock{mutex}; + observed_envs.push_back(jni::JniEnv::GetEnv()); + }); + + test_thread_1.join(); + test_thread_2.join(); + + // Placing EXPECT_EQ calls within the thread lambdas caused unexpected data + // races. Given the first thread is joined and then the second, it would seem + // impossible for this to happen, however, EXPECT_EQ doesn't make guarantees + // about the ordering of expectations (whereas join does). The write guard is + // simply to ensure writes are seen on the main thread (this is all but + // guaranteed to happen anyways, but this is technically more friendly to the + // compiler). + // + // Also, detachment is in TLS destruction, which isn't deterministic, however, + // what matters is that both complete, not really the order. + const std::vector expected_output_1{ + reinterpret_cast(0xBBBBBBBBBBB), + reinterpret_cast(0xCCCCCCCCCCC)}; + const std::vector expected_output_2{ + reinterpret_cast(0xCCCCCCCCCCC), + reinterpret_cast(0xBBBBBBBBBBB)}; + EXPECT_TRUE((observed_envs == expected_output_1 || + observed_envs == expected_output_2)); +} + +} // namespace