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!
1776ce5 to
1ffa62c
Compare
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
1ffa62c to
3d5f701
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.