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

[Brian1KB]

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);
    }
}

[garydgregory]

Hello @Brian1KB

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

[Brian1KB]

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]

    @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.

[sazzad16]

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?

[BinaryIgor]

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!

[MaciejLipinski]

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?

[garydgregory]

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.