[VL] Fix potential double unload of JNI libraries on Java 11+ #6146
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
|
Which library Velox's lib loader tries to unload? I remember that option ( |
yangzhg
changed the title
I'm not sure about which libs tries to unload, but I found after Java 11 there is no need to unload JNI libs, and the unload may cause core dump or UBs. |
Then I suppose this is not related to #6145 ? I see @zzcclp Regarding lib-unloading, why CH backend needs to unload a library? |
There are some resources need to be released when shutting jvm, otherwise ch will core dump |
apply this patch the core dump is fixed |
I will try to fix only in veloxbackend |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
@zzcclp @zhztheplayer I have made it only worked on Velox backend |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
Previously, the addHookForLibUnloading method added a shutdown hook unconditionally to unload JNI libraries. This could potentially lead to double unload issues on Java 11 and later versions where the JVM automatically handles JNI library cleanup. This commit modifies addHookForLibUnloading to conditionally add the unload hook based on the JVM version: - For Java 11 and later, the method adds a no-op hook since the JVM handles the cleanup automatically. - For Java versions prior to 11, the method adds the provided hook to ensure proper cleanup. This change ensures compatibility and prevents potential double unload issues on Java 11 and later.
|
Run Gluten Clickhouse CI |
| ShutdownHookManager.addShutdownHook(ShutdownHookManager.SPARK_CONTEXT_SHUTDOWN_PRIORITY)(hook) | ||
| ShutdownHookManager.addShutdownHook(ShutdownHookManager.SPARK_CONTEXT_SHUTDOWN_PRIORITY) { | ||
| if ( | ||
| BackendsApiManager.getBackendName == "velox" && SystemUtils.isJavaVersionAtLeast( |
There was a problem hiding this comment.
if (BackendsApiManager.getBackendName == "...")
Seems like a writing we should avoid in core module's code. Let's figure out if there is a better way.
|
@yangzhg I could not reproduce the unloading error since the static storage
For different build configurations I assume this list should either be empty since we always pass Would you like to collect some log or debug info to see which library is being unloaded in your case? Let's find out the root cause prior to doing any code changes. Thank you very much. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks. |
What changes were proposed in this pull request?
Previously, the addHookForLibUnloading method added a shutdown hook unconditionally to unload JNI libraries. This could potentially lead to double unload issues on Java 11 and later versions where the JVM automatically handles JNI library cleanup.
This commit modifies addHookForLibUnloading to conditionally add the unload hook based on the JVM version:
This change ensures compatibility and prevents potential double unload issues on Java 11 and later.
Fixes: #6145