Repository navigation
GH-1385: Match the JNI return type when closing exported C Data - #1390
hutiefang76 wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Is all of this really necessary? It's not already tested?
There was a problem hiding this comment.
You’re right, the existing tests cover the callbacks needed for this fix. I checked RoundtripTest#testSchema and StreamTest#getSchemaError separately on OpenJ9 with -Xcheck:jni: both abort with the original JNI library and pass with the two-line fix. I’ve removed ReleaseTest; the PR now changes only the two JNI calls. The affected reactor also passes clean verify with the existing 110 C Data tests.
AI-generated response, checked against the executed test results.
|
CI failure is #1393 |
|
@hutiefang76 please rebase. |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
LGTM!
close() returns void, so CallVoidMethod is the correct JNI call.
Invoking it through CallObjectMethod was undefined behavior.
I checked jni_wrapper.cc and the two calls you changed are the only ones, so nothing else needs the same fix.
Thanks for catching this!
Use CallVoidMethod for PrivateData.close in array/schema and stream release callbacks. Keep cleanup intact and exercise all three exported structures, including close failures and nested retained buffers. Generated-by: DeepSeek V4 Flash and Codex
Remove the duplicate ReleaseTest suite after confirming the existing schema and stream tests each reproduce the JNI return-type error on OpenJ9 and pass with CallVoidMethod. Generated-by: Codex and DeepSeek V4 Flash
1776ce5 to
1ffa62c
Compare
What's Changed
The C Data release callbacks invoke
PrivateData.close()throughCallObjectMethod, although its JNI descriptor is()V. With OpenJ9's JNI checks enabled, releasing an exported array, schema or stream can abort the JVM. UseCallVoidMethodin both callbacks.The existing C Data tests already exercise the affected callbacks, so this PR adds no new test suite.
Validation:
-Xcheck:jni: the existingRoundtripTest#testSchemaandStreamTest#getSchemaErroreach abort withJVMJNCK046Eagainst the unchanged JNI library (exit 87, atreleaseSchemaandreleaseArrayStreamrespectively). Both pass against the fixed library without JNI check errors.clean verifypassed: 110 C Data cases, no failures or skips; vector and memory dependencies passed as well.Closes #1385.
AI-generated PR description. Investigation, implementation and automated review assisted by Codex and DeepSeek.