Potential class loading deadlock when call static methods of Loader or Pointer

[tnatschl]

We observed a problem when calling a utility function like Loader.isLoadLibraries (which calls Loader.load() when this is the first access) in one thread while running e.g. Pointer.maxBytes() in another thread:

// thread 2
public class PrintMaxBytes implements Runnable {
	@Override
	public void run() {
		long l = org.bytedeco.javacpp.Pointer.maxBytes();
		System.out.println(l);
	}
}
var t = new Thread(new PrintMaxBytes ());
t.start();

// thread 1(calls Loader.load())
Loader.isLoadLibraries();

• let thread 2 start class loading / static init block in Pointer up until, BUT excluding Pointer line 521.
• then let thread 1 run to execute the real Loader.load(); triggered by Loader.isLoadLibraries()
• -> deadlock!

The reason seems to be that in the JNI code there is

jclass cls = (jclass)env->CallStaticObjectMethodA(JavaCPP_getClass(env, 0), putMemberOffsetMID, args);

which basically calls the static method Loader.putMemberOffset and by doing so triggers another call to Loader.load() for which the other thread already has a lock.

In our case we solved the issued by doing e.g. new Pointer(0) in an early part of the code where we are sure we still have only one thread.

[saudet]

Class.forName(..., false, ...) gets called in Loader.putMemberOffset explicitly to avoid having to load anything. Where does Loader.load() gets triggered from the latter??

[saudet]

I'm not able to reproduce this. Please set the "org.bytedeco.javacpp.logger.debug" system property to "true" to get more information on the console.

[saudet]

There is a Class.forName(..., true, ...) call on this line in load() though that loads classes from your libraries:
https://github.com/bytedeco/javacpp/blob/1.5.9/src/main/java/org/bytedeco/javacpp/Loader.java#L1289
So depending on how your libraries get loaded, that could cause a deadlock, but it's not because of JavaCPP. You need to figure out how to make your libraries thread-safe too. JavaCPP isn't going to make your libraries magically thread-safe.

[tnatschl]

Hi Samuel!

Thanks for the quick response.

To make things clearer I have prepared a quick screen recording which shows the problem we encounter: https://www.youtube.com/watch?v=eUQKkgXjHz8.

However, it may actually be "just" a problem of the combination of javacpp 1.5.6 and OpenJDK 11.

[saudet]

Thanks! I was able to reproduce the issue, thanks. The problem is that the JNI loading code needs to initialize the Pointer class, but if that is already waiting after Loader.load(), it never finishes. What can we do about this 🤔

[tnatschl]

Obviously I'm not the expert on the internals of javacpp.
However, could pulling out these kind of utility functions into a class which does not have to deal with library loading help?
I guess it would not completely solve the issue but at least reduce the risk of running into it ...

[thomas-k-git]

Maybe lock ordering? If every java class in the lib would try to class-load Pointer first and then only the Pointer class would do the actual native loading?

[saudet]

I think I've been able to fix this in commit e9b82ce
Please give it a try with the snapshots: http://bytedeco.org/builds/

[tnatschl]

I can confirm that the following program consistently runs through with version 1.5.10-20240122.132915-217:

public class Main {

    public static void main(String[] args) {
        System.err.println("JVM: " + getProperty("java.vendor") + " " + getProperty("java.version"));
        // thread 2
        var thread2 = new Thread(new PrintMaxBytes(), "Pointer-Access");
        // if called here it causes a deadlock
        thread2.start();

        // main thread calls Loader.load()
        System.err.println("calling Loader.isLoadLibraries()");
        boolean isLoaded = Loader.isLoadLibraries();
        System.err.println("isLoaded=" + isLoaded);

        // if called here it causes no deadlock
        // thread2.start();
    }

    static class PrintMaxBytes implements Runnable {

        @Override
        public void run() {
            System.err.println("calling Pointer.maxBytes()");
            long maxBytes = Pointer.maxBytes();
            System.err.println("maxBytes=" + maxBytes);
        }
    }
}

And to be on the safe side I also implemented a JCStress test like so:

import static org.openjdk.jcstress.annotations.Expect.ACCEPTABLE;
import static org.openjdk.jcstress.annotations.Expect.FORBIDDEN;

import org.bytedeco.javacpp.Loader;
import org.bytedeco.javacpp.Pointer;
import org.openjdk.jcstress.annotations.Actor;
import org.openjdk.jcstress.annotations.Description;
import org.openjdk.jcstress.annotations.JCStressTest;
import org.openjdk.jcstress.annotations.Outcome;
import org.openjdk.jcstress.annotations.State;
import org.openjdk.jcstress.infra.results.LL_Result;

public class JavaCppConcurrencyTests {

    private JavaCppConcurrencyTests() {
    }

    @JCStressTest
    @Description("Thread safety of static methods of JavaCpp")
    @Outcome(id = "1, 1", expect = ACCEPTABLE, desc = "Both actors finished and returned 1.")
    @Outcome(expect = FORBIDDEN, desc = "An actor did not return.")
    @State
    public static class StaticFunctionsDoNotDeadLock {

        @Actor
        public void actor1(LL_Result r) {
            Pointer.maxBytes();
            r.r1 = 1;
        }

        @Actor
        public void actor2(LL_Result r) {
            Loader.isLoadLibraries();
            r.r2 = 1;
        }
    }
}

for which I got a nice green result:

[tnatschl]

BTW with JavaCPP 1.5.6 this JCStress test shows that calling Loader.isLoadLibraries() in a single threaded part of the program makes concurrency between following calls to these methods unproblematic:

import static org.openjdk.jcstress.annotations.Expect.ACCEPTABLE;
import static org.openjdk.jcstress.annotations.Expect.FORBIDDEN;

import org.bytedeco.javacpp.Loader;
import org.bytedeco.javacpp.Pointer;
import org.openjdk.jcstress.annotations.Actor;
import org.openjdk.jcstress.annotations.Description;
import org.openjdk.jcstress.annotations.JCStressTest;
import org.openjdk.jcstress.annotations.Outcome;
import org.openjdk.jcstress.annotations.State;
import org.openjdk.jcstress.infra.results.LL_Result;

public class JavaCppConcurrencyTests {

    private JavaCppConcurrencyTests() {
    }

    @JCStressTest
    @Description("Thread safety of static methods of JavaCpp")
    @Outcome(id = "1, 1", expect = ACCEPTABLE, desc = "Both actors finished and returned 1.")
    @Outcome(expect = FORBIDDEN, desc = "An actor did not return.")
    @State
    public static class StaticFunctionsDoNotDeadLock {

        public StaticFunctionsDoNotDeadLock() {
            // single threaded call where all native libraries get loaded
            Loader.isLoadLibraries();
        }

        @Actor
        public void actor1(LL_Result r) {
            Pointer.maxBytes();
            r.r1 = 1;
        }

        @Actor
        public void actor2(LL_Result r) {
            Loader.isLoadLibraries();
            r.r2 = 1;
        }
    }

}

[saudet]

Right, the issue happens on class initialization only

[saudet]

Fix released with version 1.5.10. Thanks for reporting!

[tnatschl]

Thanks for acting so quickly!