Skip to content

Commit

Permalink
Shatter JvmRefBase and ThreadGuard into separate files.
Browse files Browse the repository at this point in the history
This change has no effect.

PiperOrigin-RevId: 595901454
  • Loading branch information
jwhpryor authored and copybara-github committed Jan 8, 2024
1 parent 71ef014 commit 0f0c77c
Show file tree
Hide file tree
Showing 8 changed files with 530 additions and 348 deletions.
52 changes: 48 additions & 4 deletions implementation/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand All @@ -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",
Expand All @@ -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",
],
)
Expand Down Expand Up @@ -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"],
Expand Down
4 changes: 4 additions & 0 deletions implementation/forward_declarations.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
105 changes: 2 additions & 103 deletions implementation/jvm_ref.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
45 changes: 45 additions & 0 deletions implementation/jvm_ref_base.h
Original file line number Diff line number Diff line change
@@ -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_
115 changes: 115 additions & 0 deletions implementation/jvm_ref_test.cc
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 0f0c77c

Please sign in to comment.