JDK-8332154 : Memory leak in SynchronousQueue
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 21.0.2,21.0.3,22
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2024-05-13
  • Updated: 2024-06-11
  • Resolved: 2024-05-20
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21 JDK 23
21.0.5-oracleFixed 23 b24Fixed
Related Reports
Relates :  
Relates :  
Description
ADDITIONAL SYSTEM INFORMATION :
OpenJDK 21.0.2, reproducible on Linux/MacOS. It is likely reproducible on Windows too, although our reproducer fails for unrelated reasons there.

A DESCRIPTION OF THE PROBLEM :
Since the introduction of this commit https://github.com/openjdk/jdk/commit/8d1ab57065c7ebcc650b5fb4ae098f8b0a35f112, SynchronousQueue sometimes appears in a state where it has only request nodes (i.e. it is empty) and a reference to an item it stored before.

We believe this is a memory leak: one consequence of this bug is an Executor which holds a reference to a terminated runnable. This reference prevents the runnable from being garbage-collected.

REGRESSION : Last worked in version 21

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
We wrote a test case.
This test uses a SycnhronousQueue in an environment with a lot of threads, eventually leaving it in a state with no data nodes and multiple request nodes. Then the test checks the presence of references to the previously stored objects.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Since the SynchronousQueue is empty, it should not reference the data that was added to it previously.
ACTUAL -
Sometimes this test fails on OpenJDK 21.0.2.

---------- BEGIN SOURCE ----------
import java.lang.ref.WeakReference;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.*;
import java.util.concurrent.*;
import java.util.concurrent.atomic.AtomicInteger;

/**
 * If `SynchronousQueue` is empty, then it should not reference any objects that were in it before.
 */
public class Main {

    // There are fewer suppliers that waiters, which means that all the supplied items should be consumed
    private static final int SUPPLIERS = 490;
    private static final int WAITERS = 500;
    private static final AtomicInteger waitersLocked = new AtomicInteger(0);
    private static final AtomicInteger threadsFinished = new AtomicInteger(0);
    private static final Set<Object> visitedItems = new HashSet<>();

    private static final class Leak {}

    private static final class Waiter extends Thread {
        private final BlockingQueue<Leak> queue;

        private Waiter(BlockingQueue<Leak> queue) {
            this.queue = queue;
        }

        public void run() {
            for (int i = 0; i < 100; ++i) {
                try {
                    waitersLocked.incrementAndGet();
                    // the object collected here is not reachable anymore, it should be garbage-collected
                    queue.take();
                    waitersLocked.decrementAndGet();
                } catch (InterruptedException ignored) {
                }
            }
            threadsFinished.incrementAndGet();
        }
    }

    private static class Supplier extends Thread {
        private final BlockingQueue<Leak> queue;

        private Supplier(BlockingQueue<Leak> queue) {
            this.queue = queue;
        }

        public void run() {
            for (int i = 0; i < 100; ++i) {
                try {
                    Leak leak = new Leak();
                    queue.put(leak);
                } catch (InterruptedException ignored) {
                }
            }
            threadsFinished.incrementAndGet();
        }
    }

    private static List<Field> getAllFields(Class<?> clazz) {
        ArrayList<Field> fields = new ArrayList<>();
        Collections.addAll(fields, clazz.getDeclaredFields());
        var superClass = clazz.getSuperclass();
        if (superClass != null) {
            fields.addAll(getAllFields(superClass));
        }
        return fields;
    }

    private static void introspect(Object object) throws IllegalAccessException {
        for (Field field : getAllFields(object.getClass())) {
            if (Modifier.isStatic(field.getModifiers()))  {
                continue;
            }
            field.setAccessible(true);
            Object item = field.get(object);
            if (item instanceof Leak) {
                System.out.println("Found leak");
                System.exit(1);
            }
            if (item instanceof Thread) {
                continue;
            }
            if (item != null && visitedItems.add(item)) {
                introspect(item);
            }
        }
    }

    public static void main(String[] args) throws InterruptedException, IllegalAccessException {
        SynchronousQueue<Leak> queue = new SynchronousQueue<>(false);
        for (int i = 0; i < WAITERS; ++i) {
            new Waiter(queue).start();
            if (i < SUPPLIERS) {
                new Supplier(queue).start();
            }
        }
        while (threadsFinished.get() + waitersLocked.get() != SUPPLIERS + WAITERS) {
            // spin wait
        }
        if (!queue.isEmpty()) {
            System.out.println("Queue is not empty");
            System.exit(1);
        }
        introspect(queue);
        System.out.println("Successful!");
        System.exit(0);
    }
}
---------- END SOURCE ----------

FREQUENCY : often



Comments
[jdk21u-fix-request] Approval Request from Aleksey Shipilëv Resolves the memory leak introduced in 22 and backported to 21.0.2. Applies cleanly, all tests pass. The change is simple, and seems to be what `LinkedTransferQueue` already does here: https://github.com/openjdk/jdk/blob/b25476200ab8bea4f25a671d5b9351662d11c5b4/src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java#L616-L617
10-06-2024

A pull request was submitted for review. URL: https://git.openjdk.org/jdk21u-dev/pull/687 Date: 2024-06-10 16:07:05 +0000
10-06-2024

A pull request was submitted for review. URL: https://git.openjdk.org/jdk22u/pull/237 Date: 2024-06-03 08:58:50 +0000
03-06-2024

A pull request was submitted for review. URL: https://git.openjdk.org/jdk21u-dev/pull/624 Date: 2024-05-30 09:36:07 +0000
03-06-2024

Changeset: b78613b6 Author: Viktor Klang <vklang@openjdk.org> Date: 2024-05-20 18:52:34 +0000 URL: https://git.openjdk.org/jdk/commit/b78613b6813a85662fb2af2004d0b68002fe471d
20-05-2024

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/19271 Date: 2024-05-16 14:54:52 +0000
16-05-2024

[~dl] Preliminary info is that it is the last item added which leaks in the end, so it would seem like the `item` is not unlinked in the final dequeue.
15-05-2024

[~dl] From a first glance it it probably the case that a node needs to null out its element field? Let's discuss.
15-05-2024