Skip to content

fix: wrap CountDownLatch.await() in assertTrue() in tests - #20071

Open
zhang-arvin wants to merge 1 commit into
apache:masterfrom
zhang-arvin:fix/9285-countdownlatch-assert
Open

fix: wrap CountDownLatch.await() in assertTrue() in tests#20071
zhang-arvin wants to merge 1 commit into
apache:masterfrom
zhang-arvin:fix/9285-countdownlatch-assert

Conversation

@zhang-arvin

Copy link
Copy Markdown

Purpose

Wrap CountDownLatch.await(timeout, unit) calls in assertTrue() to prevent silent test timeouts. When the boolean return value of await() is ignored, a timeout causes the test to silently pass instead of failing.

Changes

  • PostJoinCursorTest.java: wrap countDownLatch.await(1, TimeUnit.SECONDS) in assertTrue()
  • CoordinatorSegmentMetadataCacheTest.java: wrap latch.await(1, TimeUnit.SECONDS) in Assert.assertTrue()
  • ServerManagerTest.java: wrap 4 occurrences of factory.notifyLatch.await(1000, TimeUnit.MILLISECONDS) in Assert.assertTrue()
  • JettyTest.java: wrap latch.await(5, TimeUnit.SECONDS) in Assert.assertTrue()

Closes #9285

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

Reviewed 4 of 4 changed files.

The review found one test-thread lifecycle issue that can hang the JVM after a timeout.


This is an automated review by Codex GPT-5.6-Luna(max)

joinCursorThread.start();

countDownLatch.await(1, TimeUnit.SECONDS);
assertTrue(countDownLatch.await(1, TimeUnit.SECONDS));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Timeout assertion can skip interrupt cleanup

If await returns false, the assertion throws before joinCursorThread.interrupt(). The intentionally infinite, non-daemon test thread can then continue indefinitely and hang the test JVM. Move interruption and joining into cleanup that runs before or alongside the assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dangling CountDownLatch.await() in tests

2 participants