Improving loops in BallThread.java & updating its README.md. #3212
Conversation
PR SummaryThis PR improves the efficiency of the 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 (2)
Files Processed (3)
- localization/es/map-reduce/README.md (1 hunk)
- twin/README.md (4 hunks)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (3 hunks)
Actionable Comments (3)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [73-55]
possible bug: "Potential for missed wakeup signals in
suspendMe()method." -
twin/src/main/java/com/iluwatar/twin/BallThread.java [78-80]
possible bug: "Potential for missed wakeup signals in
resumeMe()method." -
twin/src/main/java/com/iluwatar/twin/BallThread.java [84-87]
possible bug: "Potential for missed wakeup signals in
stopMe()method."
Skipped Comments (0)
| while (isRunning) { | ||
| while (isSuspended) { | ||
| try { | ||
| wait(); // Puts the thread in waiting state. |
There was a problem hiding this comment.
The wait() method should be called inside a loop to handle spurious wakeups. The current implementation might miss wakeup signals, leading to unexpected behavior.
| public synchronized void resumeMe() { | ||
| isSuspended = false; | ||
| LOGGER.info("Begin to resume BallThread"); | ||
| notify(); // Wakes up the thread from waiting state. |
There was a problem hiding this comment.
The notify() method should be called inside a loop to handle spurious wakeups. The current implementation might miss wakeup signals, leading to unexpected behavior.
| public synchronized void stopMe() { | ||
| isRunning = false; | ||
| isSuspended = false; | ||
| notify(); // Makes sure the thread wakes up and exits (stops). |
There was a problem hiding this comment.
The notify() method should be called inside a loop to handle spurious wakeups. The current implementation might miss wakeup signals, leading to unexpected behavior.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- dd45cb0: Synchronization updated thanks to GitHub Actions.
Files Processed (2)
- twin/README.md (4 hunks)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (3 hunks)
Actionable Comments (1)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [51-60]
possible bug: "Potential deadlock in BallThread.java"
Skipped Comments (0)
| synchronized (this) { | ||
| while (isRunning) { | ||
| while (isSuspended) { | ||
| try { | ||
| wait(); // Puts the thread in waiting state. | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); // Restores the interruption. | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The while (isSuspended) loop in run() lacks a check for isRunning. If isRunning becomes false while the thread is suspended, the thread will never terminate. Add a check for isRunning inside the while (isSuspended) loop to ensure proper termination.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 0d2cdb1: Updated to GitHub Actions comments.
Files Processed (2)
- twin/README.md (4 hunks)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (3 hunks)
Actionable Comments (1)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [51-60]
possible bug: "Potential race condition in thread termination."
Skipped Comments (1)
-
twin/README.md [8-245]
enhancement: "Review README.md additions for clarity."
| synchronized (this) { | ||
| while (isRunning) { | ||
| while (isSuspended && isRunning) { | ||
| try { | ||
| wait(); // Puts the thread in waiting state. | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); // Restores the interruption. | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The while (isSuspended && isRunning) loop in run() method lacks a check for isRunning after the wait() call. This could lead to the thread continuing to run even after it's been stopped.
|
The build is failing. Please ensure that all the checks pass before requesting review. |
Pull Request Template
What does this PR do?
This pull request solves the issue of unnecessary CPU usage, as it also updates the README.md, of the file
BallThread.java.Changes apply to Issue #2977 (Fix busy-waiting loops); already existing issue regarding the resources consumption due to inefficient loops.
All the best,
Francisco!