"BC Entropy Daemon" does not exit when interrupted

[fwiesweg]

This is problematic e.g. when exec-maven-plugin interrupts all threads of a given execution and joins them to cleanly terminate the custom code before continuing the build. The join times out after 15secs, but it's nonetheless a noticeable and unnecessary delay for all builds in a CI pipeline and fills the log with bogus warnings. This probably happens in any VM that (willingly or not) runs the the relevant bouncy castle setup code.

This is caused by the restructuring done by commit 552b8e3 in the following file:

https://github.com/bcgit/bc-java/blob/master/prov/src/main/java/org/bouncycastle/jcajce/provider/drbg/DRBG.java

The actual fix should be quite simple, something like what follows, but should probably be checked by the original authors (@dghgit ) to validate it.

    private static class EntropyDaemon
        implements Runnable
    {

/** shortened */

        @Override
        public void run()
        {
            for (; ; )
            {
                Runnable task = tasks.pollFirst();

                if (task != null)
                {
                    try
                    {
                        task.run();
                    }
+                   catch (InterruptedException e) 
+                   {
+                        Thread.currentThread().interrupt();
+                        return;
+                   }
                    catch (Throwable e)
                    {
                        // ignore
                    }
                }
                else
                {
                    try
                    {
                        Thread.sleep(5000);
                    }
                    catch (InterruptedException e)
                    {
                        Thread.currentThread().interrupt();
+                       return;
                    }
                }
            }
        }
    }

[dghgit]

Thanks for the report, interesting... try what's now on https://www/bouncycastle,org.betas 173b01 or later and let us know how it goes.

[fwiesweg]

I just tested it and it works fine, thanks a lot!

[vlsi]

@dghgit , Can you expose a clean API to stop all bc-related threads?

As of now, the only way to find the BC thread is to enumerate all the threads which is a poor option.

At the same time, can you please make BC Entropy Daemon optional? (==disabled by default)
The current "BC Entropy Daemon" does prevent class unloading, and it does cause obscure "OutOfMemory: Metaspace" issues.

[vlsi]

Yet another alternative could be reusing Java's Executors.newSingleThreadExecutor or something like that.
Then the thread class does not really hold direct references to bouncycastle classes, so BC classes can be unloaded.

However, it would require certain extra coding so the background thread can detect BC unloading and stop itself (e.g. PhantomReference)

[dghgit]

I guess the first step is to move the EntropyDaemon out of the provider class so the additional classes don't hang around.

Detecting that the originating object is unloaded seems like a trickier issue though - can you explain what you mean in reference to PhantomReference?

It would be better to get it working with the thread enabled, I suspect there will still be platforms that need it where there isn't good access to an entropy source. Optionally disabling it is a possibility as well though.

[vlsi]

I guess the first step is to move the EntropyDaemon out of the provider class so the additional classes don't hang around.

It won’t help: class holds reference to classloader, and classloader holds references to all its classes. So if you keep a strong reference to a single class, then you prevent unloading of all the classes in the classloader.

[vlsi]

Detecting that the originating object is unloaded seems like a trickier issue though - can you explain what you mean in reference to PhantomReference?

See PhantomReference in https://www.infoworld.com/article/2073891/java-se-trash-talk-part-2.html

[vlsi]

It would be better to get it working with the thread enabled, I suspect there will still be platforms that need it where there isn't good access to an entropy source

Can the actions be executed within the caller thread?

[dghgit]

So I think I might have a solution. I've tried to approach it in 2 ways, I've moved the internal definitions for the thread so in theory a lot more will get unloaded first up and then the garbage should cause the entropy thread to exit when it collects the original DRBG class that invoked the entropy thread,

Try what's on https://www.bouncycastle.org/betas 173b11 or later. Let me know how it goes. Thanks.

[vlsi]

Can you share the code for the changes?

Could you add a test that the class actually unloads?
See https://github.com/classloader-leak-test-framework/README.md, https://github.com/pgjdbc/pgjdbc/blob/f61fbfe7b72ccf2ca0ac2e2c366230fdb93260e5/pgjdbc/src/test/java/org/postgresql/test/jdbc4/jdbc41/SharedTimerClassLoaderLeakTest.java#L27

[dghgit]

code changes should now be on github. See https://github.com/bcgit/bc-java/tree/master/prov/src/main/java/org/bouncycastle/jcajce/provider/drbg

still working on a test, a further note, testing this properly seems to be a lot of work - more importantly whether something unloads properly or not depends a lot on the custom class loader involved. You'll need to try it locally for us to be able to determine this actually fixes the problem.

[vlsi]

You'll need to try it locally for us to be able to determine this actually fixes the problem.

Based on the code above, I would say it does not fix the problem.

See

https://docs.oracle.com/javase/specs/jls/se9/html/jls-12.html#jls-12.7
A class or interface may be unloaded if and only if its defining class loader may be reclaimed by the garbage collector as discussed in §12.6.

EntropyDaemon class would prevent unloading of the class loader, so parentClassMonitor.get() will never return null.

[dghgit]

hmmm. So what does it do? The class loader you're talking about this time is already behaving in a non-standard way - a provider jar is normally loaded by a system class loader, by the definition above this problem would never be solvable.

[vlsi]

I load bc-java from a custom classloader, so bc classes are not loaded by the system classloader.

The unloading is possible in case none of bc-java classes are referenced from gc roots. At the same time, an active thread that references bc class would prevent unloading.

[dghgit]

Okay, this isn't quite how the conversation started... I think the only way out of this is to give you the option of not having the thread. Try what's up on https://www.bouncycastle.org/betas 173b14 or later. If you set "org.bouncycastle.drbg.no_thread" to "true" it should avoid starting the thread but still keep things seeded. Let me know how it goes.

[vlsi]

Can you make "no_thread" a default behavior?
I can't always control JVM flags, especially, I can't control JVM properties when I use bc-java within a library.

I think the only way out of this is to give you the option of not having the thread

Not having a thread is one of the solutions.

Another solution is to avoid dependency on bc-java classes in a thread.
My second comment on this thread mentions exactly that alternative (see Executors.newSingleThreadExecutor): #1254 (comment)

[dghgit]

hmmm.... Would you tell me if it at least works as is? We have tried using Executors.newSignleThreadExecutor in the past, but it did not go very well.

[vlsi]

https://www.bouncycastle.org/betas

The link does not open for me.
Would you push a snapshot build to Maven Central?

[dghgit]

hmmm. Try https://downloads.bouncycastle.org/betas/

[vlsi]

The error is ERR_CONNECTION_TIMED_OUT