Copilot commented on code in PR #12302:
URL: https://github.com/apache/gluten/pull/12302#discussion_r3419951815
##########
cpp/core/jni/JniCommon.h:
##########
@@ -549,3 +550,86 @@ class JavaRssClient : public RssClient {
jmethodID javaPushPartitionData_;
jbyteArray array_;
};
+
+/// Bridges gluten::ThreadInitializer callbacks to a Java-side
+/// NativeThreadInitializer instance via JNI.
+///
+/// On initialize(), attaches the current native thread to the JVM and calls
+/// into the Java initializer so it can install Spark TaskContext or other
+/// thread-local state. On destroy(), calls the Java destroy() method but
+/// does NOT detach the thread — the underlying JVM thread may be reused by
+/// the pool, and detaching prematurely could allow the Java Thread object
+/// to be garbage-collected.
+class SparkThreadInitializer final : public gluten::ThreadInitializer {
+ public:
+ /// @param vm The JavaVM pointer from JNI_OnLoad.
+ /// @param jInitializerLocalRef A local reference to a Java object
+ /// implementing org.apache.gluten.threads.NativeThreadInitializer.
+ /// A global reference is created internally.
+ SparkThreadInitializer(JavaVM* vm, jobject jInitializerLocalRef) : vm_(vm) {
+ JNIEnv* env;
+ attachCurrentThreadAsDaemonOrThrow(vm_, &env);
+ jInitializerGlobalRef_ = env->NewGlobalRef(jInitializerLocalRef);
+ GLUTEN_CHECK(jInitializerGlobalRef_ != nullptr, "Failed to create global
reference for native thread initializer.");
+ (void)initializeMethod(env);
+ }
+
+ SparkThreadInitializer(const SparkThreadInitializer&) = delete;
+ SparkThreadInitializer(SparkThreadInitializer&&) = delete;
+ SparkThreadInitializer& operator=(const SparkThreadInitializer&) = delete;
+ SparkThreadInitializer& operator=(SparkThreadInitializer&&) = delete;
+
+ ~SparkThreadInitializer() override {
+ JNIEnv* env;
+ if (vm_->GetEnv(reinterpret_cast<void**>(&env), jniVersion) != JNI_OK) {
+ LOG(WARNING) << "SparkThreadInitializer#~SparkThreadInitializer(): "
+ << "JNIEnv was not attached to current thread";
+ return;
+ }
+ env->DeleteGlobalRef(jInitializerGlobalRef_);
+ }
Review Comment:
If the destructor runs on a thread not attached to the JVM, it returns early
and leaks `jInitializerGlobalRef_`. Consider attaching in the destructor (or
otherwise ensuring a valid `JNIEnv*`) and deleting the global ref reliably;
since this is a destructor, avoid throwing (e.g., attach and log on failure).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
