reactor / BlockHound

Java agent to detect blocking calls from non-blocking threads.
Apache License 2.0
1.33k stars 97 forks source link

IllegalMonitorStateException in ReentrantLock$Sync.tryRelease on executor shutdown #38

Open szpak opened 5 years ago

szpak commented 5 years ago

Working on JMH in #35 I have encountered an exception thrown on an executor service shutdown if ReactorThreadFactory is used with rejectBlocking: false (and daemon: false in that case):

OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
result
java.lang.IllegalMonitorStateException
    at java.base/java.util.concurrent.locks.ReentrantLock$Sync.tryRelease(ReentrantLock.java:149)
    at java.base/java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1302)
    at java.base/java.util.concurrent.locks.ReentrantLock.unlock(ReentrantLock.java:439)
    at java.base/java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:440)
    at java.base/java.util.concurrent.ThreadPoolExecutor.getTask(ThreadPoolExecutor.java:1054)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1114)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:834)

It occurs only with BlockHound installed. Sample code to produce that result:

public class ReproduceIllegalMonitorStateException {

    public static void main(String[] args) throws ExecutionException, InterruptedException {
        ThreadFactory blockingUnfriendlyThreadFactory = new ReactorThreadFactory("blocking-unfriendly", new AtomicLong(), false, true,
                (t, e) -> e.printStackTrace());
        ExecutorService executor = Executors.newSingleThreadExecutor(blockingUnfriendlyThreadFactory);;
        try {
            BlockHound.install();
            System.out.println(executor.submit(() -> "result").get());
        } finally {
            executor.shutdown(); //fails with non daemon thread with "java.lang.IllegalMonitorStateException" on "ReentrantLock$Sync.tryRelease()"
        }
    }
}
bsideup commented 5 years ago

Hi @szpak,

Thanks for reporting! 👍 I confirm that there is a blocking call detected:

java.lang.Exception: Blocking call: jdk.internal.misc.Unsafe#park
    at reactor.core.scheduler.Blah.lambda$main$1(Blah.java:20)
    at reactor.blockhound.BlockHound$Builder.lambda$install$8(BlockHound.java:259)
    at reactor.blockhound.BlockHoundRuntime.checkBlocking(BlockHoundRuntime.java:43)
    at java.base/jdk.internal.misc.Unsafe.park(Unsafe.java)
    at java.base/java.util.concurrent.locks.LockSupport.park(LockSupport.java:194)
    at java.base/java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2081)
    at java.base/java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:433)
    at java.base/java.util.concurrent.ThreadPoolExecutor.getTask(ThreadPoolExecutor.java:1054)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1114)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:835)

Although since this is ThreadPoolExecutor's internals, I'm keen to whitelist it. WDYT?

FYI sometimes exceptions get swallowed (like here), but you can use this trick:

BlockHound.install(b -> {
    b.blockingMethodCallback(m -> {
        new Exception("Blocking call: " + m).printStackTrace();
    });
});
szpak commented 5 years ago

Yes, in the meantime I also got the root cause of that issue (with blockingMethodCallback()) and I just wanted to paste the same stacktrace :).

Regarding whitelisting, as my case is legitimate - Callable submitted from a normal thread - I don't see any other clear way to do not break that code.

szpak commented 5 years ago

The whitelisting has disadvantage that I cannot simply test non-blocking thread, no "markers" in the stacktrace scenario :). In general, in production code, all of the blocking calls in non-blocking threads will be ignored if run though a executor.

bsideup commented 5 years ago

You can test it without the executor. In fact, I would not use the executor here (nor any Reactor's code)

szpak commented 5 years ago

On the other hand, whitelisting allowBlockingCallsInside("java.util.concurrent.ThreadPoolExecutor", "getTask"); probably not all the calls will be ignored.

Update. Response to myself. GH didn't refresh the page.

szpak commented 5 years ago

You can test it without the executor. In fact, I would not use the executor here (nor any Reactor's code)

Currently I have my own ThreadFactory and I do not reuse Reactor's code. However, it's handy to use single thread ThreadPoolExecutor to reuse one thread in thousand of calls (within one JMH iteration). What do you propose to use instead?