Remove use of finalizers in JNA and improve concurrency for `Memory`, `CallbackReference` and `NativeLibrary` by matthiasblaesing · Pull Request #1402 · java-native-access/jna

matthiasblaesing · 2021-12-28T14:01:43Z

No description provided.

matthiasblaesing · 2021-12-28T15:56:12Z

The finalizer mechanism is deprecated for removal: https://mail.openjdk.java.net/pipermail/jdk-dev/2021-November/006279.html
So while #finalize was warned about for a long time, now removal must happen.

dbwiddis · 2022-04-12T22:40:04Z

FYI, I've got a report of running out of memory that I'm pretty sure is related to excessive use of finalizer in the Memory allocations underlying HANDLEByReference and Structure classes when reading process and thread performance counters. I'll copy over the new Memory code and attempt to test the code and quantify the improvement, but I personally am now a lot more interested in this PR than I was before.

matthiasblaesing · 2022-04-19T20:09:12Z

@dbwiddis - I reread the changes and the API is still intact. There is a potential behavioral change, but I think it should not be common that people overrode #dispose and expected it to be called from #finalize. It won't get better, so it would be nice if you could give it another look.

dbwiddis

LGTM, but a few comments/suggestions.

dbwiddis · 2022-04-19T22:43:19Z

+                        if(ref instanceof CleanerRef) {
+                            ((CleanerRef) ref).clean();
+                        }
+                    } catch (InterruptedException ex) {


Should we set Thread.currentThread.interrupt() here? I recognize that the thread will essentially terminate on an interrupt and it probably doesn't matter in practice, but will keep static analysis tools happier.

I don't get the intention. InterruptedException is raised if the waiting thread is interrupted (in this case the cleaner thread). Why would an analyser expect that the threads interrupts itself again?

Analyzers don't like when you catch an interrupted exception and do nothing. The general guidance is to either re-throw the exception or re-set the interrupted flag so that any upstream code recognizes the interruption.

As I said it probably doesn't matter in this case as the thread just terminates. (I'm not sure if it would terminate with a different exit code with that flag or not, or if that matters.)

This article says what I'm saying a lot better than I can: https://programming.guide/java/handling-interrupted-exceptions.html

matthiasblaesing · 2022-04-21T19:55:40Z

Ok - this needs more work - the unittests became flaky. I first thought, that the cleaner implementation of ProxyObject was broken, but that does not explain unrelated failures. I suspect, that the Memory objects get quicker collected and thus we see race effects.

matthiasblaesing · 2022-04-23T14:47:58Z

This is the problematic part:

    [junit] Exception in thread "JNA Cleaner" java.lang.Error: Invalid memory access
    [junit] 	at com.sun.jna.Native._getPointer(Native Method)
    [junit] 	at com.sun.jna.Native.getPointer(Native.java:2263)
    [junit] 	at com.sun.jna.Pointer.getPointer(Pointer.java:642)
    [junit] 	at com.sun.jna.platform.win32.COM.COMInvoker._invokeNativeInt(COMInvoker.java:37)
    [junit] 	at com.sun.jna.platform.win32.COM.Unknown.Release(Unknown.java:80)
    [junit] 	at com.sun.jna.platform.win32.COM.util.ProxyObject$Disposer.run(ProxyObject.java:747)
    [junit] 	at com.sun.jna.internal.Cleaner$CleanerRef.clean(Cleaner.java:127)
    [junit] 	at com.sun.jna.internal.Cleaner$1.run(Cleaner.java:60)
    [junit] Testsuite: com.sun.jna.platform.win32.COM.util.HybdridCOMInvocationTest

I still try to understand how we could end there.

dbwiddis · 2022-04-24T17:34:31Z

    [junit] 	at com.sun.jna.platform.win32.COM.util.ProxyObject$Disposer.run(ProxyObject.java:747)

I suspect a race condition in ObjectFactory.disposeAll() (called in testOfficeInvocationProblemCOMUtil() in the problem test suite) which calls ProxyObject.dispose() (which no longer synchronizes on anything but is called by a method synchronizing on the factory's registered objects), vs. ProxyObject.Disposer.run() which synchronizes on the Disposer object itself.

dbwiddis · 2022-04-27T19:59:06Z

FYI, long-running program with the Memory portion of this PR resolved the out-of-memory issues seen earlier with too many finalizers. So if we can resolve the test failures this looks like a good improvement.

matthiasblaesing · 2022-05-05T19:55:57Z

So what happened: I removed the changes to the platform code, added stabilization to some of the regularly failing tests to get noise down or get better diagnostics. Multiple runs got through cleanly or reported errors that are "consistent" with breaks, that can happen in a shared system.

I intent to merge this at the end of the week end.

The following uses of finalizers were replaced by a Cleaner or removed because they are unneeded: - c.s.j.Memory#finalize - c.s.j.CallbackReference#finalize - c.s.j.NativeLibrary#finalize The cleanup handling is moved to the #close method and #dispose is being deprecated. Anyone relying on #dispose being called from the GC has to take action now! If a subclass needs a cleanup after this object goes out of scope, it will need to register a cleaner.

…UnloadFromResourcePath on Intel 32bit

…if test fails

matthiasblaesing force-pushed the remove_finalizer branch 2 times, most recently from fca09be to 77755e1 Compare December 28, 2021 14:25

matthiasblaesing mentioned this pull request Dec 28, 2021

High monitor usage due to Finalizer.add #1353

Closed

dbwiddis mentioned this pull request Apr 12, 2022

Excessive finalizer use in PerfCounterQueryHandler windows environment oshi/oshi#2009

Closed

dbwiddis mentioned this pull request Apr 13, 2022

Replace Finalizers with Cleaners for PDH Query Handles oshi/oshi#2010

Closed

matthiasblaesing force-pushed the remove_finalizer branch 3 times, most recently from 79c98e0 to 7a2387b Compare April 19, 2022 19:59

dbwiddis approved these changes Apr 19, 2022

View reviewed changes

matthiasblaesing force-pushed the remove_finalizer branch from 9a64627 to be1a2d0 Compare April 23, 2022 12:25

matthiasblaesing force-pushed the remove_finalizer branch 6 times, most recently from edb7975 to cb5be82 Compare May 5, 2022 18:24

matthiasblaesing added 5 commits May 10, 2022 19:10

Skip JNALoadTest#testLoadAndUnloadFromJar and JNALoadtest#testLoadAnd…

357b6df

…UnloadFromResourcePath on Intel 32bit

Report failure from Kernel32NamedPipeTest#testMultiThreadedNamedPipe …

ef1e56e

…if test fails

Report resultCode on failing expectations

099abb8

Content of WeakReference needs to be null checked before usage

cebf3ac

matthiasblaesing force-pushed the remove_finalizer branch from cb5be82 to cebf3ac Compare May 10, 2022 17:10

matthiasblaesing merged commit a8faaec into java-native-access:master May 10, 2022

matthiasblaesing deleted the remove_finalizer branch May 16, 2022 18:34

dbwiddis mentioned this pull request Jun 28, 2022

Using Memory#close can throw NPE if cleanable not initialized #1446

Closed

Conversation

matthiasblaesing commented Dec 28, 2021

Uh oh!

matthiasblaesing commented Dec 28, 2021

Uh oh!

dbwiddis commented Apr 12, 2022

Uh oh!

matthiasblaesing commented Apr 19, 2022

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

dbwiddis Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

matthiasblaesing Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

dbwiddis Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthiasblaesing commented Apr 21, 2022

Uh oh!

matthiasblaesing commented Apr 23, 2022

Uh oh!

dbwiddis commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbwiddis commented Apr 27, 2022

Uh oh!

matthiasblaesing commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbwiddis commented Apr 24, 2022 •

edited

Loading