fix: replace Thread.sleep() with synchronization in TmuxEncodingTest#1827
Conversation
Use CountDownLatch.await() for pane thread keep-alive and Object.wait() for output polling, fixing SonarCloud S2925.
📝 WalkthroughWalkthroughA test class is modified to improve synchronization by introducing a latch mechanism that replaces unconditional thread sleep with a wait condition. The polling loop now uses synchronized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builtins/src/test/java/org/jline/builtins/TmuxEncodingTest.java`:
- Around line 77-79: The synchronized wait on masterOut (synchronized
(masterOut) { masterOut.wait(100); }) is ineffective because nothing notifies
that monitor (ByteArrayOutputStream.write never calls notify), so replace the
pattern with a real event-driven signal or an explicit poll: either swap the
ByteArrayOutputStream instance (masterOut) for a small subclass/wrapper that
overrides write(...) to synchronize on masterOut and call notifyAll() after
writing so the existing synchronized wait becomes meaningful, or remove the
wait/notify dance and use a clear polling helper (e.g., Awaitility or a
short-loop with Thread.sleep and a timeout) around the contains("世") check;
touch the code paths involving masterOut, paneTerminal.writer().print/flush, and
the LineDisciplineTerminal output to ensure the writer signals the monitor if
you choose the notify-on-write approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2e07a1d-545e-4898-aea4-30906c0ce25d
📒 Files selected for processing (1)
builtins/src/test/java/org/jline/builtins/TmuxEncodingTest.java
| synchronized (masterOut) { | ||
| masterOut.wait(100); | ||
| } |
There was a problem hiding this comment.
masterOut.wait(100) is effectively a Thread.sleep(100) — nothing ever notifies this monitor.
Nothing in the code path (paneTerminal.writer().print/flush → LineDisciplineTerminal → ByteArrayOutputStream.write) calls masterOut.notify() / notifyAll(). ByteArrayOutputStream's write methods do not notify waiters. As a result, every iteration will block the full 100 ms and the loop exits only via the 3 s deadline or the contains("世") check — exactly the same behavior as the previous Thread.sleep(100), just dressed up to silence SonarCloud S2925. A future reader will reasonably assume there is a producer-side notification and may be misled when modifying this code.
If the goal is a real event-driven wait, let the writer actually signal the monitor, e.g. by swapping ByteArrayOutputStream for a subclass that notifies on write:
♻️ Option A: notify on write so `wait(100)` is meaningful
- ByteArrayOutputStream masterOut = new ByteArrayOutputStream();
+ ByteArrayOutputStream masterOut = new ByteArrayOutputStream() {
+ `@Override`
+ public synchronized void write(int b) {
+ super.write(b);
+ notifyAll();
+ }
+ `@Override`
+ public synchronized void write(byte[] b, int off, int len) {
+ super.write(b, off, len);
+ notifyAll();
+ }
+ };With this in place the existing synchronized (masterOut) { masterOut.wait(100); } becomes a genuine latency-reducing wakeup instead of a fixed delay.
♻️ Option B: keep intent explicit with a well-known helper (e.g. Awaitility) or a small polling utility
Avoids the sleep-vs-wait debate entirely and documents the polling intent clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builtins/src/test/java/org/jline/builtins/TmuxEncodingTest.java` around lines
77 - 79, The synchronized wait on masterOut (synchronized (masterOut) {
masterOut.wait(100); }) is ineffective because nothing notifies that monitor
(ByteArrayOutputStream.write never calls notify), so replace the pattern with a
real event-driven signal or an explicit poll: either swap the
ByteArrayOutputStream instance (masterOut) for a small subclass/wrapper that
overrides write(...) to synchronize on masterOut and call notifyAll() after
writing so the existing synchronized wait becomes meaningful, or remove the
wait/notify dance and use a clear polling helper (e.g., Awaitility or a
short-loop with Thread.sleep and a timeout) around the contains("世") check;
touch the code paths involving masterOut, paneTerminal.writer().print/flush, and
the LineDisciplineTerminal output to ensure the writer signals the monitor if
you choose the notify-on-write approach.
1 participant
Summary
Thread.sleep(2000)withCountDownLatch.await()for pane thread keep-aliveThread.sleep(100)withObject.wait(100)for output pollingSummary by CodeRabbit