commons-pool icon indicating copy to clipboard operation
commons-pool copied to clipboard

Java virtual threads locked up / parking / permanently blocking -- swap synchronized block for reentrant lock

Open Brian1KB opened this issue 1 year ago • 11 comments

This PR resolves an actively occurring issue with virtual threads that is easily reproducible. If you call borrowObject within the GenericObjectPool at a high rate in a short period of time with virtual threads, all of the threads within the ForkJoinPool for virtual threads will lock up & park, and no virtual threads will resume.

I've already seen there is another Loom related PR that has existed for over a year, and I hope that including this demonstration of a production issue will hope speed up the process for getting this resolved. The demo will print "attempting to acquire", but never "acquired". If you apply my PR, this issue is resolved -- in addition, it passes all existing unit tests.

I'm confident that there are other similar issues within the project, though I would sure imagine that the maintainers would have a better idea of where to look for such problems. Additionally, I'm confident this is not the correct branch to PR to, and if I could have I would've filed an issue instead & left this part to more competent hands, but the sign up process for the JIRA was too complex, and figuring out how to use a mailing list might be more complex for me than debugging this issue was.

package ie.briankenny.main;

import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
import org.apache.commons.pool2.impl.DefaultPooledObject;
import org.apache.commons.pool2.impl.GenericObjectPool;
import org.apache.commons.pool2.impl.GenericObjectPoolConfig;

import java.util.concurrent.Executor;
import java.util.concurrent.Executors;

public class Main {
    public static void main(String[] args) throws Exception {
        GenericObjectPoolConfig poolConfig = new GenericObjectPoolConfig();

        poolConfig.setMinIdle(4);
        poolConfig.setMaxIdle(4);
        poolConfig.setMaxTotal(8);

        GenericObjectPool<DummyObject> pool = new GenericObjectPool<>(new DummyObjectFactory());

        Executor executor = Executors.newVirtualThreadPerTaskExecutor();

        for (int i = 0; i < 1000; i++) {
            executor.execute(() -> {
                try {
                    System.out.println("attempting to acquire");

                    DummyObject dummyObject = pool.borrowObject();

                    System.out.println("acquired");

                    pool.returnObject(dummyObject);
                } catch (Exception e) {
                    throw new RuntimeException(e);
                }
            });
        }

        while (true) {
            Thread.sleep(100);
        }
    }
}

class DummyObject {
    public DummyObject() throws InterruptedException {
        long sleepTime = (long) (Math.random() * 1000);

        Thread.sleep(sleepTime);
    }
}

class DummyObjectFactory extends BasePooledObjectFactory<DummyObject> {
    @Override
    public DummyObject create() throws Exception {
        return new DummyObject();
    }

    @Override
    public PooledObject<DummyObject> wrap(DummyObject obj) {
        return new DefaultPooledObject<>(obj);
    }
}

Brian1KB avatar Aug 21 '24 02:08 Brian1KB

Hello @Brian1KB

Thank you for your report. Please add a unit test to this PR.

garydgregory avatar Aug 21 '24 12:08 garydgregory

Hello @Brian1KB

Thank you for your report. Please add a unit test to this PR.

To add a unit test, I'd need to include a newVirtualThreadPerTaskExecutor, however the project is targeted at a much earlier version of Java.

Brian1KB avatar Aug 21 '24 21:08 Brian1KB

    @Test
    @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
    public void testVirtualThreadLoad() throws Exception {
        BasePooledObjectFactory<String> slowObjectFactory = createSlowObjectFactory(500);

        Executor executor = Executors.newVirtualThreadPerTaskExecutor();

        int executions = 1000;

        CountDownLatch countDownLatch = new CountDownLatch(executions);

        final GenericObjectPool<String> pool = new GenericObjectPool<>(slowObjectFactory);

        for (int i = 0; i < executions; i++) {
            executor.execute(() -> {
                try {
                    String borrowed = pool.borrowObject();

                    pool.returnObject(borrowed);

                    countDownLatch.countDown();
                } catch (Exception e) {
                    throw new RuntimeException(e);
                }
            });
        }

        countDownLatch.await();

        pool.close();
    }

Untested, but I'd expect something like this would suffice.

Brian1KB avatar Aug 21 '24 21:08 Brian1KB

Hi @garydgregory, could you or anyone else please help/guide @Brian1KB about tests?

On a side note, this PR changes the locking mechanism to use Java Lock objects instead of synchronized blocks. IMHO, the changes are already being covered by existing unit tests. Would it be possible to consider this PR without specific unit tests as the changes are being tested anyway?

sazzad16 avatar Sep 05 '24 09:09 sazzad16

Is there anything I can help with to make it merged and released? We are eager to use Jedis with Virtual Threads and we cannot do this without this fix and then a release of Jedis with this dependency updated!

BinaryIgor avatar Sep 20 '24 17:09 BinaryIgor

I second @sazzad16 's plea for assistance – @garydgregory could this be merged without additional unit tests, as this behavior is already covered by preexisting tests?

MaciejLipinski avatar Oct 17 '24 11:10 MaciejLipinski

Hello all and @psteitz

My view here is twofold:

  • No unit tests: Without a unit test that fails if the changes to main are not applied, this is just an invitation for a regression, or new bugs. What I don't know is if you can use a multi release source folder with tests or if its only supported for main. That would solve the issue of wanting a test that uses a post Java 8 API.
  • Inconsistent use of locks. It would be best if the whole library used the same style of locking.

garydgregory avatar Oct 17 '24 12:10 garydgregory

Hi everyone, I'd like to know if there has been any progress on this issue? I'm encountering this problem on JDK 21, causing my platform threads to expand infinitely. https://github.com/adoptium/adoptium-support/issues/1319 https://mail.openjdk.org/pipermail/loom-dev/2025-July/thread.html

funky-eyes avatar Jul 02 '25 09:07 funky-eyes

Hi @funky-eyes

See my previous comments. With JEP 491 in JDK 24, a significant improvement has been made regarding virtual threads and synchronized blocks. Virtual threads can now acquire, hold, and release monitors independently of their carriers.

How is your use case on Java 24?

garydgregory avatar Jul 02 '25 14:07 garydgregory

Hi @funky-eyes

See my previous comments. With JEP 491 in JDK 24, a significant improvement has been made regarding virtual threads and synchronized blocks. Virtual threads can now acquire, hold, and release monitors independently of their carriers.

How is your use case on Java 24?

Hi garydgregory

First, thank you for your reply. Second, our application upgrades require strict evaluation and testing, so this is quite time-consuming and labor-intensive work. Moreover, JDK 24 is not an LTS version, and we need to wait for JDK 25. Although I have already verified in JDK 24 that there are no pinned threads, I believe the community should consider users on JDK 21. Furthermore, based on the scope of changes in this PR, I don't think it would negatively impact the core code.

funky-eyes avatar Jul 03 '25 01:07 funky-eyes

So this not an issue anymore in JDK 24?

robvanmaris avatar Aug 11 '25 09:08 robvanmaris