Fixed busy waiting in points 1,2,3,7 #3101
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…busy-looping---Alhusseain
|
iluwatar
requested changes
Jan 11, 2025
microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java
Outdated
Show resolved
Hide resolved
queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java
Show resolved
Hide resolved
server-session/src/main/java/com/iluwatar/sessionserver/App.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| twin.draw(); | ||
| twin.move(); | ||
| Thread.sleep(250); |
There was a problem hiding this comment.
This Thread.sleep(250) is the main problem. Instead of using a thread, we could utilize the same aforementioned ScheduledExecutorService
There was a problem hiding this comment.
On it, Thanks for the feedback
There was a problem hiding this comment.
I have implemented all requested changes except only in BallThread.java. The only change was removing sleep since it is unnecessary. In BallThread.java, There is no need to run it every fixed amount of time. Instead, waking the thread up only when there is a request to resume and making it wait when there is a request to suspend is sufficient to reduce overhead. This way, I implement event-based logic as requested in the issue.
…busy-looping---Alhusseain
PR SummaryThis PR addresses busy-waiting issues in several Java applications by implementing wait/notify mechanisms. The changes prevent unnecessary overhead in threads by making them wait until work is available, improving efficiency and resource utilization. This involved adding synchronization primitives and modifying thread execution logic across multiple files. 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 (23)
- fa902ad: Merge remote-tracking branch 'origin/busy-looping---Alhusseain' into busy-looping---Alhusseain
- 1c04548: implemented review changes
- 18a4c02: Merge branch 'master' into busy-looping---Alhusseain
- b5073d6: LogoutHandlerTest correct
- 7afa9c8: LogoutHandlerTest correct
- 4751cb8: Add Service Executor Start and Wake State Test
- d0e0d87: Add Service Executor Start and Wake State Test
- 30d6eb3: Add BallThread resume and suspend state test
- 7be96ca: Add Expiration Task state test (should be sleeping after it is woken)
- e31b12b: Add Expiration Task state test (should be sleeping after it is woken)
- 986130a: Add Expiration Task state test (should be waiting at the start)
- fca409f: Add Expiration Task state test (should be waiting at the start)
- a8fb4b2: Merge remote-tracking branch 'origin/busy-looping---Alhusseain' into busy-looping---Alhusseain
- d84dc96: SonarQube fix try 4 (Reliability and coverage in twin)
- ffd4773: Merge branch 'master' into busy-looping---Alhusseain
- ec0d423: SonarQube fix try 3
- 992d899: SonarQube fix try 2
- 2109002: SonarQube fix try 1
- 27325f2: Checkstyle fix try 3
- 42e4c32: Checkstyle fix try 2
- 9415119: Checkstyle fix try 1
- 58e5c62: Fixed busy waiting in points 1,2,3,7 (1)
- f20a582: Fixed busy waiting in points 1,2,3,7
Files Processed (15)
- microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java (4 hunks)
- microservices-log-aggregation/src/test/java/com/iluwatar/logaggregation/LogAggregatorTest.java (1 hunk)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/App.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/MessageQueue.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/TaskGenerator.java (1 hunk)
- queue-based-load-leveling/src/test/java/com/iluwatar/queue/load/leveling/TaskGenSrvExeTest.java (2 hunks)
- server-session/README.md (2 hunks)
- server-session/pom.xml (1 hunk)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (3 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LoginHandler.java (2 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LogoutHandler.java (1 hunk)
- twin/README.md (2 hunks)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (1 hunk)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (4 hunks)
Actionable Comments (1)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [78-84]
possible bug: "Potential deadlock in thread synchronization."
Skipped Comments (0)
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- dbf6a7b: implemented review changes
Files Processed (14)
- microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java (4 hunks)
- microservices-log-aggregation/src/test/java/com/iluwatar/logaggregation/LogAggregatorTest.java (1 hunk)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/App.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/MessageQueue.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/TaskGenerator.java (1 hunk)
- queue-based-load-leveling/src/test/java/com/iluwatar/queue/load/leveling/TaskGenSrvExeTest.java (2 hunks)
- server-session/pom.xml (1 hunk)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (3 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LoginHandler.java (2 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LogoutHandler.java (1 hunk)
- twin/README.md (2 hunks)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (1 hunk)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (5 hunks)
Actionable Comments (1)
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [91-108]
bug: "Infinite loop in session expiration task."
Skipped Comments (0)
server-session/src/main/java/com/iluwatar/sessionserver/App.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 9671120: Merge branch 'master' into busy-looping---Alhusseain
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
…busy-looping---Alhusseain
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (1)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (3 hunks)
Actionable Comments (0)
Skipped Comments (1)
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [87-88]
enhancement: "Improve scheduler shutdown handling."
|
|
I'm sorry, but it doesn't look correct. The solution needs a complete rewrite, so I'll close this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close issue #2977
Implemented wait and notify functionalities on threads that have busy waiting issues
No. 1 : Server Session / App.java
sessionExpirationTask now waits until at least a single login request is sent, since there would be unnecessary overhead if it runs with no login requests available.
No. 2 : Twin / BallThread.java
run() now waits if suspendMe is called, and resumes if resumeMe is called, since there would be unnecessary overhead if run() is running when ball item is suspended. ( no need to always check if suspended)
No. 3: Log Aggregation / LogAggregator.java
startBufferFlusher runs if there is at least one log is present, similiar to No.1's fix
No. 7: Queue-Based Load Leveling / ServiceExecutor.java
similiar to No.3 and No.1, at least a single msg is added to queue for it to work, until then run() waits.
I did not refactor No. 4,5,6 since I think no unnecessary overhead is present ( as the retry pattern implemented loops until certain number of errors occur, doesn't loop needlessly)
I would much appreciate your feedback, which I would consider and act upon quickly.