8332154: Memory leak in SynchronousQueue

[viktorklang-ora]

Local testing seems to indicate that this fix (which mirrors what's done in the FIFO mode) addresses the problem. Regression test added for JDK20+

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8332154: Memory leak in SynchronousQueue (Bug - P3)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19271/head:pull/19271
$ git checkout pull/19271

Update a local copy of the PR:
$ git checkout pull/19271
$ git pull https://git.openjdk.org/jdk.git pull/19271/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19271

View PR using the GUI difftool:
$ git pr show -t 19271

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19271.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vklang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@viktorklang-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332154: Memory leak in SynchronousQueue

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 77 new commits pushed to the master branch:

• d6b7f9b: 8331851: Add specific regression leap year tests for Calendar.roll()
• b92bd67: 8332403: Anachronistic reference to Netscape Communicator in Swing API docs
• 8acdd2d: 8330565: C2: Multiple crashes with CTW after JDK-8316991
• 0a58cff: 8298405: Implement JEP 467: Markdown Documentation Comments
• 39a55e9: 8324809: compiler can crash with SOE while proving if two recursive types are disjoint
• b7ae0ae: 8328572: JFR: Use Class.forPrimitiveName(String)
• e611151: 8331281: RISC-V: C2: Support vector-scalar and vector-immediate bitwise logic instructions
• 44bdf99: 8332239: Improve CSS for block tags
• 9bb6169: 8317621: --add-script should support JavaScript modules
• 4eb1eaf: 8329617: Update stylesheet for specs and tool documentation
• ... and 67 more: https://git.openjdk.org/jdk/compare/5053b70a7fc67ce9b73dbeecbdd88fbc34d45e04...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@viktorklang-ora The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (e0b2eb89)
• 03: Full - Incremental (f32e9ea4)
• 02: Full - Incremental (cc0a2014)
• 01: Full - Incremental (f365c5f0)
• 00: Full (dfab08e8)

[viktorklang-ora]

@AlanBateman @DougLea Reviews are most welcome :)

[knisht]

@viktorklang-ora We managed to reproduce it quite reliably by running the test from the bug report multiple times (in our case, it was 100). I understand this does not make a 100% stable regression reproducer, but in case of multiple runs the probability of false positive in quite low.

[vprovodin]

Hi @viktorklang-ora
The changes was verified as follows

1. Create the script launching the test (from bug description) in a loop and calculating the number of failures.

cat <<EOF >runme.sh
#!/bin/bash -x

__test_jdk=\$1
__test=\$2
__count=\${3:-20}

i=0
failures=0
while true; do

  ((i=i+1))
  echo i: \$i

  \$__test_jdk/bin/java \
      --add-opens=java.base/java.util.concurrent=ALL-UNNAMED \
      --add-opens=java.base/java.lang=ALL-UNNAMED \
      \$__test || break

  if [ "\$i" -ge "\$__count" ]; then
    break
  fi
done
EOF

2. Launch the test as follows

bash runme.sh ${test_jdk_home} Main 100

When the test is launched 100 times it is guaranteed to fail in at least one iteration. With this fix no failures were detected in 1000 runs.

[DougLea]

Looks good; thanks. I don't know why I left out this check in a case that is otherwise identical to LTQ.

[viktorklang-ora]

@DougLea I decided to move to using VirtualThreads and therefor pulling the memory leak test out into its own class which only runs for JDK20+.

[DougLea]

Sure. Seems reasonable.

[viktorklang-ora]

@DougLea Perfect, thanks for reviewing! 👍

[AlanBateman]

Looks okay, I just wonder how reliable assertDoesntLeak with concurrent agents, debug builds, and VM options such as a Xcomp that will challenge the await(10s). Minimally the timed awaits should be untimed. Another idea is try an ES like this:

        try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
            for (int i = 0; i < NUMBER_OF_ITEMS; i++) {
                executor.submit( .. producer .. );
                executor.submit(.. consumer ..);
            }
        }

No need for the count down latches or the need to retry on InterruptedException. The close method waits for the 400 virtual threads to finish so you can assert that the queue is empty.

The loop that waits for survivors to empty can be unbounded too. If there is a leak then the test will timeout.

[viktorklang-ora]

@AlanBateman Great suggestions, Alan. I've updated the PR to reflect your improvements.

[viktorklang-ora]

/integrate

[openjdk]

Going to push as commit b78613b.
Since your change was applied there have been 78 commits pushed to the master branch:

• 7652f98: 8331885: C2: meet between unloaded and speculative types is not symmetric
• d6b7f9b: 8331851: Add specific regression leap year tests for Calendar.roll()
• b92bd67: 8332403: Anachronistic reference to Netscape Communicator in Swing API docs
• 8acdd2d: 8330565: C2: Multiple crashes with CTW after JDK-8316991
• 0a58cff: 8298405: Implement JEP 467: Markdown Documentation Comments
• 39a55e9: 8324809: compiler can crash with SOE while proving if two recursive types are disjoint
• b7ae0ae: 8328572: JFR: Use Class.forPrimitiveName(String)
• e611151: 8331281: RISC-V: C2: Support vector-scalar and vector-immediate bitwise logic instructions
• 44bdf99: 8332239: Improve CSS for block tags
• 9bb6169: 8317621: --add-script should support JavaScript modules
• ... and 68 more: https://git.openjdk.org/jdk/compare/5053b70a7fc67ce9b73dbeecbdd88fbc34d45e04...master

Your commit was automatically rebased without conflicts.

[openjdk]

@viktorklang-ora Pushed as commit b78613b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[viktorklang-ora]

@knisht @vprovodin Thanks for the thoughts, I decided to go for a blackbox test to make sure that this test does not rely on any internal impl details of SynchronousQueue.