Uh oh!
There was an error while loading. Please reload this page.
Remove JniObjectReference SafeHandle backend - #1446
Conversation
The SafeHandle-backed object reference implementation was an old experiment kept for migration and performance comparison, but the active build has long used the IntPtr-backed JniObjectReference path only. Remove the unsupported feature switches, SafeHandle reference types, excluded tests, and the obsolete SafeHandle invocation-overhead strategy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the previously experimental/unsupported SafeHandle-backed JniObjectReference implementation path so that the IntPtr representation is the sole supported backend across Java.Interop and related benchmark tooling/docs. This reduces conditional compilation branching, deletes unused SafeHandle-era types/tests, and updates the invocation-overhead benchmark to reflect only the currently supported invocation strategies.
Changes:
- Removed SafeHandle backend code paths and deleted now-dead SafeHandle reference wrapper types from
src/Java.Interop. - Removed the excluded SafeHandle-specific test and simplified test project configuration accordingly.
- Removed SafeHandle strategy support from
jnienv-gen, regeneratedtests/invocation-overhead/jni.cs, and updated docs/benchmark README to describe SafeHandle as historical.
Show a summary per file
| File | Description |
|---|---|
| tests/Java.Interop-Tests/Java.Interop/JniReferenceSafeHandleTest.cs | Deletes the SafeHandle-only test fixture. |
| tests/Java.Interop-Tests/Java.Interop-Tests.csproj | Removes the explicit compile exclusion for the deleted SafeHandle test file. |
| tests/invocation-overhead/README.md | Updates benchmark documentation to treat SafeHandle as historical and describe current strategies. |
| tests/invocation-overhead/jni.cs | Regenerates benchmark JNI bindings without the SafeHandle strategy. |
| tests/invocation-overhead/invocation-overhead.csproj | Drops the SafeHandle feature define from the benchmark project. |
| tests/invocation-overhead/invocation-overhead.cs | Removes SafeHandle benchmark implementation (SafeTiming) and related types/aliases. |
| src/Java.Interop/Java.Interop/JniWeakGlobalReference.cs | Deletes SafeHandle-backed weak-global reference wrapper type. |
| src/Java.Interop/Java.Interop/JniTransition.cs | Removes SafeHandle-only local reference frame push/pop usage. |
| src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | Removes SafeHandle-only cross-thread local-ref invalidation logic. |
| src/Java.Interop/Java.Interop/JniReferenceSafeHandle.cs | Deletes base SafeHandle wrapper type and related helpers. |
| src/Java.Interop/Java.Interop/JniPeerMembers.cs | Removes SafeHandle-only thread-safety checks for local refs. |
| src/Java.Interop/Java.Interop/JniObjectReference.cs | Makes JniObjectReference unconditionally IntPtr-backed and removes SafeHandle/feature-guarded branches. |
| src/Java.Interop/Java.Interop/JniLocalReference.cs | Deletes SafeHandle-backed local reference wrapper type. |
| src/Java.Interop/Java.Interop/JniGlobalReference.cs | Deletes SafeHandle-backed global reference wrapper type. |
| src/Java.Interop/Java.Interop/JniEnvironment.Types.cs | Removes SafeHandle-only FindClass fallback logic and clarifies unsupported build-path behavior. |
| src/Java.Interop/Java.Interop/JniEnvironment.cs | Removes SafeHandle-only local reference tracking/frame management code. |
| src/Java.Interop/Java.Interop/JniAllocObjectRef.cs | Deletes SafeHandle-only alloc-object local reference wrapper type. |
| src/Java.Interop/Java.Interop/JavaObject.cs | Removes SafeHandle backend storage; keeps only control-block (IntPtr) peer reference storage. |
| src/Java.Interop/Java.Interop/JavaException.cs | Removes SafeHandle backend storage; keeps only control-block (IntPtr) peer reference storage. |
| src/Java.Interop/Java.Interop.csproj | Removes now-obsolete FEATURE_JNIOBJECTREFERENCE_INTPTRS constant from project defines. |
| Documentation/Architecture.md | Updates architecture documentation to describe SafeHandle support as historical and the IntPtr model as supported. |
| build-tools/jnienv-gen/Generator.cs | Removes SafeHandle emission path and updates generator preprocessor logic for remaining strategies. |
Copilot's findings
- Files reviewed: 21/22 changed files
- Comments generated: 1
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
simonrozsival
commented
Jun 8, 2026
/review |
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM — Clean removal of dead code
Well-executed cleanup that removes the unused SafeHandle-backed JniObjectReference experiment. The conditional compilation blocks were consistently removed, the jnienv-gen generator was properly updated, documentation accurately describes the SafeHandle path as historical, and no references to the removed feature flags remain in src/.
Positive callouts:
- Thorough removal — all
FEATURE_JNIOBJECTREFERENCE_SAFEHANDLES,FEATURE_JNIOBJECTREFERENCE_INTPTRS, andFEATURE_JNIENVIRONMENT_SAFEHANDLESreferences are gone - The
_NAMESPACE_PER_HANDLElogic inGenerator.cswas correctly updated to detect when any two of the remaining four handle styles coexist, rather than only triggering on SafeHandle + another style - Good safety net:
JniEnvironment.Types.csnow has a#else throw NotSupportedException(...)for unsupported build configurations - Clear PR description with decision record explaining why the code was removed
One inline comment on tests/invocation-overhead/jni.cs — the #endif comment on line 8 is missing !FEATURE_JNIENVIRONMENT_JI_FUNCTION_POINTERS (the #if has four conditions, the comment only lists three). The updated Generator.cs fixes this, but jni.cs appears not to have been regenerated from it. Since it's regenerated at build time, this is low-impact.
Note (not in diff — can't comment inline):tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.Partial.cs has a HaveSafeHandles field that uses reflection to check for the now-removed gcHandle field on JniObjectReference. It will always return false now. Tests relying on it (JniTransitionTest.Dispose_ClearsLocalReferences, GetValue_ReturnsNullWithInvalidSafeHandle) will always be skipped. Consider cleaning these up in a follow-up.
Generated by Java.Interop PR Reviewer for issue #1446 · ● 12.9M
| #define FEATURE_JNIENVIRONMENT_JI_PINVOKES | ||
| #endif // !FEATURE_JNIENVIRONMENT_SAFEHANDLES && !FEATURE_JNIENVIRONMENT_JI_INTPTRS && !FEATURE_JNIENVIRONMENT_JI_PINVOKES && !FEATURE_JNIENVIRONMENT_XA_INTPTRS | ||
| #endif // !FEATURE_JNIENVIRONMENT_JI_INTPTRS && !FEATURE_JNIENVIRONMENT_JI_PINVOKES && !FEATURE_JNIENVIRONMENT_XA_INTPTRS |
There was a problem hiding this comment.
🤖 #endif comment doesn't match the #if expression. The #if on line 6 has four conditions (including !FEATURE_JNIENVIRONMENT_JI_FUNCTION_POINTERS), but this #endif comment only lists three. The updated Generator.cs (line 93) now correctly emits the full expression, but this file wasn't regenerated from it.
Since jni.cs is regenerated at build time via invocation-overhead.targets, this will fix itself on the next build, but the checked-in file should match the generator output. Consider re-running jnienv-gen to update this file.
Rule: #else/#endif comments must match the original expression
simonrozsival
commented
Jun 8, 2026
@copilot address the build failure - it appears jni.cs needs to be regenerated: |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Fixed in
|
Uh oh!
There was an error while loading. Please reload this page.
Related to dotnet/android#11843
Summary
Remove the unsupported
JniObjectReferenceSafeHandle backend and make the existingIntPtrrepresentation the only implementation:FEATURE_JNIOBJECTREFERENCE_SAFEHANDLES/FEATURE_JNIOBJECTREFERENCE_INTPTRSbranching fromJava.InteropJniReferenceSafeHandle,JniLocalReference,JniGlobalReference,JniWeakGlobalReference, andJniAllocObjectReftypesjnienv-genand regeneratetests/invocation-overhead/jni.csDecision record
The SafeHandle backend was useful when Java.Interop was exploring how JNI object references should be represented. The architecture and invocation-overhead docs show the intended goals: stronger handle separation, possible GC cleanup of leaked JNI refs, and a way to compare a safer representation against an
IntPtrrepresentation.That experiment has been rejected for the current runtime model:
IntPtr-backedJniObjectReferencepathFEATURE_JNIOBJECTREFERENCE_INTPTRSdefineJniObjectReferenceControlBlock/IntPtrmodel for runtime and GC-bridge integrationSince there are no current plans to resurrect the SafeHandle backend, keeping it adds noise and gives a false impression that the configuration is supported. Git history preserves the experiment if it is ever needed again.
Validation
dotnet build -t:Prepare -nologo -m:1dotnet build src/Java.Interop/Java.Interop.csproj -nologodotnet build build-tools/jnienv-gen/jnienv-gen.csproj -nologodotnet build tests/invocation-overhead/invocation-overhead.csproj -nologo -p:NativeToolchainSupported=falsedotnet test tests/Java.Interop-Tests/Java.Interop-Tests.csproj -nologo -p:NativeToolchainSupported=falseI also attempted
dotnet build Java.Interop.sln -nologo. The local full-solution build is blocked by unrelated environment/baseline failures, including the native link step failing withld: library 'c++' not foundand unrelated generated Java.Base/Kotlin-Gradle failures.