server-session: remove busy-wait loop in App.java using ScheduledExecutorService#3445
server-session: remove busy-wait loop in App.java using ScheduledExecutorService#3445Kaaldut8 wants to merge 4 commits intoiluwatar:masterfrom
Conversation
|
⏳ Analyzing changes in this PR... ⏳ This might take a few minutes, please wait 📥 CommitsAnalyzing changes from base (
📁 Files being considered (12)🔄 commander/src/main/java/com/iluwatar/commander/Retry.java (3 hunks) 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)
Files Processed (1)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (3 hunks)
Actionable Comments (1)
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [92-119]
possible bug: "Replace busy-wait with scheduled expiration task."
Skipped Comments (3)
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [31-35]
readability: "Concerning import style for concurrency utilities."
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [61-63]
performance: "Initialize singleton scheduler for session expiration checks."
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [87-90]
maintainability: "Graceful shutdown hook for scheduler."
| private static void sessionExpirationTask() { | ||
| new Thread( | ||
| () -> { | ||
| while (true) { | ||
| try { | ||
| LOGGER.info("Session expiration checker started..."); | ||
| Thread.sleep(SESSION_EXPIRATION_TIME); // Sleep for expiration time | ||
| Instant currentTime = Instant.now(); | ||
| synchronized (sessions) { | ||
| synchronized (sessionCreationTimes) { | ||
| Iterator<Map.Entry<String, Instant>> iterator = | ||
| sessionCreationTimes.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Instant> entry = iterator.next(); | ||
| if (entry | ||
| .getValue() | ||
| .plusMillis(SESSION_EXPIRATION_TIME) | ||
| .isBefore(currentTime)) { | ||
| sessions.remove(entry.getKey()); | ||
| iterator.remove(); | ||
| } | ||
| } | ||
| } | ||
| scheduler | ||
| .scheduleAtFixedRate(() -> { | ||
| try { | ||
| LOGGER.info("Session expiration checker started..."); | ||
| Instant currentTime = Instant.now(); | ||
| synchronized (sessions) { | ||
| synchronized (sessionCreationTimes) { | ||
| Iterator<Map.Entry<String, Instant>> iterator = | ||
| sessionCreationTimes.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Instant> entry = iterator.next(); | ||
| if (entry | ||
| .getValue() | ||
| .plusMillis(SESSION_EXPIRATION_TIME) | ||
| .isBefore(currentTime)) { | ||
| sessions.remove(entry.getKey()); | ||
| iterator.remove(); | ||
| } | ||
| LOGGER.info("Session expiration checker finished!"); | ||
| } catch (InterruptedException e) { | ||
| LOGGER.error("An error occurred: ", e); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
| }) | ||
| .start(); | ||
| } | ||
| LOGGER.info("Session expiration checker finished!"); | ||
| } catch (Exception e) { | ||
| LOGGER.error("An error occurred: ", e); | ||
| } | ||
| }, SESSION_EXPIRATION_TIME, SESSION_EXPIRATION_TIME, TimeUnit.MILLISECONDS); | ||
| } |
There was a problem hiding this comment.
Replaced busy-wait loop with a scheduled task that periodically checks for expired sessions. The code uses a fixed-rate schedule with a 10s interval and safely removes expired sessions under synchronized access to shared maps.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (3)
Files Processed (12)
- commander/src/main/java/com/iluwatar/commander/Retry.java (3 hunks)
- microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java (3 hunks)
- microservices-self-registration/eurekaserver/pom.xml (1 hunk)
- microservices-self-registration/eurekaserver/src/main/java/com/learning/eurekaserver/EurekaserverApplication.java (1 hunk)
- microservices-self-registration/eurekaserver/src/test/java/com/learning/eurekaserver/EurekaserverApplicationTests.java (1 hunk)
- pom.xml (1 hunk)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java (1 hunk)
- retry/src/main/java/com/iluwatar/retry/Retry.java (3 hunks)
- retry/src/main/java/com/iluwatar/retry/RetryExponentialBackoff.java (1 hunk)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (3 hunks)
- twin/src/main/java/com/iluwatar/twin/App.java (1 hunk)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (1 hunk)
Actionable Comments (4)
-
commander/src/main/java/com/iluwatar/commander/Retry.java [59-61]
possible bug: "Use of a shared scheduler and thread-safety concerns"
-
commander/src/main/java/com/iluwatar/commander/Retry.java [103-123]
possible bug: "Backoff retry runs on scheduler thread; potential concurrency issue"
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [97-123]
possible bug: "Replace busy-wait with scheduled expiration; potential deadlock"
-
queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java [42-57]
possible issue: "Potential busy-wait in ServiceExecutor.run"
Skipped Comments (4)
-
microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java [47-49]
maintainability: "Replace thread with scheduled executor; lifecycle"
-
microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java [108-109]
performance: "Scheduled flush; consider thread-safety"
-
twin/src/main/java/com/iluwatar/twin/App.java [50-50]
maintainability: "Use of virtual thread to start BallThread"
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [61-63]
maintainability: "Scheduler for expiration tasks; resource lifecycle"
| private static final ScheduledExecutorService scheduler = | ||
| Executors.newSingleThreadScheduledExecutor(); | ||
|
|
There was a problem hiding this comment.
Potential concurrency risk: a static singleton scheduler is used to drive retries for all Retry instances. This can cause race conditions on shared state (errors/list) if multiple invocations run in parallel. Consider per-instance scheduling or ensure proper lifecycle shutdown of the scheduler, and make shared state thread-safe.
| long testDelay = | ||
| (long) Math.pow(2, this.attempts.intValue()) * 1000 + RANDOM.nextInt(1000); | ||
| long delay = Math.min(testDelay, this.maxDelay); | ||
| Thread.sleep(delay); | ||
| } catch (InterruptedException f) { | ||
| // ignore | ||
| scheduler.schedule(() -> perform(list, obj), delay, TimeUnit.MILLISECONDS); | ||
| return; | ||
| } catch (Exception j) { | ||
| this.errors.add(j); | ||
|
|
||
| if (this.attempts.incrementAndGet() >= this.maxAttempts || !this.test.test(j)) { | ||
| this.handleError.handleIssue(obj, j); | ||
| return; | ||
| } | ||
|
|
||
| long testDelay = | ||
| (long) Math.pow(2, this.attempts.intValue()) * 1000 + RANDOM.nextInt(1000); | ||
|
|
||
| long delay = Math.min(testDelay, this.maxDelay); | ||
|
|
||
| scheduler.schedule(() -> perform(list, obj), delay, TimeUnit.MILLISECONDS); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This backoff now uses a scheduled task to retry. Watch for concurrency: the same 'list' and 'errors' structures are captured and reused by the scheduled task, which may execute in parallel with the caller thread. If 'errors' is not thread-safe, this may cause concurrent modification exceptions or data races. Recommend making 'errors' a thread-safe list (e.g., CopyOnWriteArrayList) or guarding modifications with synchronization. Also consider whether a per-attempt scheduling model is intended to share a single scheduler across all retries; if not, scope the scheduler to the Retry instance.
| .scheduleAtFixedRate(() -> { | ||
| try { | ||
| LOGGER.info("Session expiration checker started..."); | ||
| Instant currentTime = Instant.now(); | ||
| synchronized (sessions) { | ||
| synchronized (sessionCreationTimes) { | ||
| Iterator<Map.Entry<String, Instant>> iterator = | ||
| sessionCreationTimes.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Instant> entry = iterator.next(); | ||
| if (entry | ||
| .getValue() | ||
| .plusMillis(SESSION_EXPIRATION_TIME) | ||
| .isBefore(currentTime)) { | ||
| sessions.remove(entry.getKey()); | ||
| iterator.remove(); | ||
| } | ||
| LOGGER.info("Session expiration checker finished!"); | ||
| } catch (InterruptedException e) { | ||
| LOGGER.error("An error occurred: ", e); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
| }) | ||
| .start(); | ||
| } | ||
| LOGGER.info("Session expiration checker finished!"); | ||
| } catch (Exception e) { | ||
| LOGGER.error("An error occurred: ", e); | ||
| } | ||
| }, SESSION_EXPIRATION_TIME, SESSION_EXPIRATION_TIME, TimeUnit.MILLISECONDS); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The session expiration task is now scheduled with a fixed-rate executor. It eliminates the busy-wait loop in main and uses a synchronized block around the two maps. Potential deadlock risk exists due to nested synchronization on separate locks; consider consolidating to a single lock or using a concurrent map approach to avoid nested locks.
| @Override | ||
| public void run() { | ||
| try { | ||
| while (!Thread.currentThread().isInterrupted()) { | ||
|
|
||
| var msg = msgQueue.retrieveMsg(); | ||
|
|
||
| if (null != msg) { | ||
| LOGGER.info(msg + " is served."); | ||
| } else { | ||
| LOGGER.info("Service Executor: Waiting for Messages to serve .. "); | ||
| if (msg != null) { | ||
| LOGGER.info("{} is served.", msg); | ||
| } | ||
|
|
||
| Thread.sleep(1000); | ||
| } | ||
| } catch (Exception e) { | ||
| LOGGER.error(e.getMessage()); | ||
| LOGGER.error("Error processing message", e); | ||
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The ServiceExecutor now implements Runnable and uses a loop that calls retrieveMsg. If retrieveMsg is non-blocking, this can lead to a busy-wait. Ensure retrieveMsg blocks or insert a small sleep/backoff to yield CPU when no messages are available; also consider handling InterruptedException to exit cleanly.
1 participant
Summary
This PR removes busy-waiting loops and improves thread interruption handling across multiple modules in the repository.
Busy-waiting and ignored interruptions can lead to inefficient CPU usage and prevent graceful thread shutdown. These changes improve concurrency behavior and align the implementation with recommended Java concurrency practices.
Changes Made
Removed busy-waiting loops and improved interruption handling in the following locations:
Key Improvements
Thread.sleep()polling loopsThread.currentThread().interrupt())InterruptedExceptionImpact
These changes: