fix: replace busy waiting in BallThread - #3615
Sakthinatchiyar wants to merge 6 commits into
Conversation
PR SummaryReplaced busy-waiting in BallThread with a wait/notify-based suspension mechanism. Introduced a dedicated Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- c6a0096: fix: replace busy waiting in BallThread
Files Processed (1)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (2 hunks)
Actionable Comments (1)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [49-56]
best_practice: "Graceful handling of InterruptedException during wait"
Skipped Comments (3)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [43-43]
best_practice: "Introduce a dedicated lock for suspend/resume coordination"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [79-85]
best_practice: "Correct use of wait/notify in resumeMe"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [88-93]
maintainability: "Graceful termination synchronization in stopMe"
| synchronized (lock) { | ||
| while (isSuspended && isRunning) { | ||
| try { | ||
| lock.wait(); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wait-for-resume loop uses the lock to suspend, and handle InterruptedException gracefully. The current approach rethrows InterruptedException as a RuntimeException, which may crash the thread. Recommend re-interrupt and exit gracefully.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 625f3c4: style: format BallThread
Files Processed (1)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (2 hunks)
Actionable Comments (0)
Skipped Comments (5)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [52-54]
best_practice: "Graceful handling of InterruptedException in wait loop."
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [82-82]
best_practice: "Use notifyAll for waking waiting threads."
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [91-91]
best_practice: "Wake waiting threads on stop."
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [65-65]
enhancement: "Conditional sleep to align with running state."
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [73-75]
maintainability: "Ensure suspendMe uses lock for visibility."
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3615 +/- ##
============================================
- Coverage 84.06% 84.06% -0.01%
+ Complexity 4353 4352 -1
============================================
Files 1133 1133
Lines 15400 15417 +17
Branches 739 740 +1
============================================
+ Hits 12946 12960 +14
Misses 2158 2158
- Partials 296 299 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 8682c01: test: cover BallThread stop while suspended
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [118-139]
possible issue: "Flaky test risk due to timing"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5a2d0e9: fix: handle BallThread interruption gracefully
Files Processed (1)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (2 hunks)
Actionable Comments (0)
Skipped Comments (3)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [49-58]
best_practice: "Wait/notify suspension loop uses a dedicated lock"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [89-93]
enhancement: "Stop path wake behavior when thread is sleeping"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [52-56]
best_practice: "Graceful handling of InterruptedException in wait loop"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 0bf6651: test: cover BallThread interruption while suspended
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [118-139]
maintainability: "Unreliable timing in testStopWhileSuspended."
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [141-162]
maintainability: "Unreliable timing in testInterruptWhileSuspended."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 92803fb: test: wait for suspended BallThread state
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [118-145]
possible bug: "Potential flakiness when detecting suspension via Thread.State.WAITING."
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [147-168]
possible bug: "Interruption handling during suspension could be fragile if interruption state is swallowed."
This branch has not been deployed
1 participant
What does this PR do?
This PR replaces the busy-waiting behavior in
BallThreadwithwait/notifyfor thread suspension and resumption.Changes
wait().notify()when resuming the thread.stopMe()to safely wake the thread when stopping.Tests
.\mvnw.cmd -pl twin testRelated to #2977