fix: handle awaitTermination result and ensure proper ExecutorService shutdown #3244

2897robo · 2025-04-07T04:23:05Z

What does this PR do?

This pull request addresses a high severity issue (java:S2095) reported by SonarCloud in the App.java file of the leader-followers module.

The issue was:

"Use try-with-resources or close this 'ExecutorService' in a 'finally' clause."
(See SonarCloud rule java:S2095)

What was the problem?

The ExecutorService created using Executors.newFixedThreadPool(...) was not guaranteed to shut down properly, which could lead to:

Resource leakage
Threads not terminating gracefully
Reliability issues in long-running systems

What does this PR change?

Wrapped ExecutorService usage inside a try-finally block
Handled the return value of awaitTermination(...) to check whether the shutdown was completed within the timeout
Added a fallback shutdownNow() in the finally block
This fix is in line with the best practices for managing ExecutorService lifecycle in Java

Why not try-with-resources?

ExecutorService does not implement AutoCloseable, so it cannot be used with try-with-resources.
Hence, we use try-finally as the proper alternative here.

References

Rule: java:S2095 - Resources should be closed
SonarCloud issue: AZXOQpt67L73unJ7u3kh

✅ Fixes: #2865
🧹 Impact: Improves resource handling, stability, and SonarCloud quality gates

… shutdown - Added handling for the result of awaitTermination to avoid Sonar warning - Wrapped ExecutorService with try-finally for proper shutdown (java:S2095) - Prevents potential resource leak and aligns with best practices Fixes: iluwatar#2865 Note: ExecutorService is not AutoCloseable, so try-with-resources is not applicable. Used try-finally instead.

github-actions · 2025-04-07T04:23:18Z

PR Summary

This PR addresses a high severity issue (java:S2095) reported by SonarCloud by improving resource handling and ensuring proper ExecutorService shutdown in the App.java file. It wraps ExecutorService usage inside a try-finally block, handles awaitTermination return value, and adds a fallback shutdownNow().

Changes

File	Summary
`leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java`	The `ExecutorService` is now properly shut down using a `try-finally` block. The `awaitTermination` result is handled to check for successful shutdown within the timeout, and a fallback `shutdownNow()` is used if necessary. The logger is also defined explicitly using `LoggerFactory.getLogger(...)`.

autogenerated by presubmit.ai

github-actions

✅ LGTM!

Review Summary

Commits Considered (2)

c217d2d: Merge branch 'iluwatar:master' into master
634e77a: fix: handle awaitTermination result and ensure proper ExecutorService shutdown
Added handling for the result of awaitTermination to avoid Sonar warning
Wrapped ExecutorService with try-finally for proper shutdown (java:S2095)
Prevents potential resource leak and aligns with best practices

Fixes: #2865

Note: ExecutorService is not AutoCloseable, so try-with-resources is not applicable. Used try-finally instead.

Files Processed (1)

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java (1 hunk)

Actionable Comments (0)

Skipped Comments (1)

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java [80-82]

possible issue: "Unnecessary shutdownNow() call."

2897robo · 2025-04-07T04:40:09Z

Hi @iluwatar, kindly reminding for review when you get a chance 🙂
This PR fixes a SonarCloud high severity issue and passed all checks.
Thanks in advance!

iluwatar · 2025-04-07T17:58:10Z

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java

+      addTasks(taskSet);
+      boolean terminated = exec.awaitTermination(2, TimeUnit.SECONDS);
+      if (!terminated) {
+        System.out.println("Executor did not terminate in the given time.");


Use Lombok's @slf4j annotation for logging

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

0f78c56: fix: add missing logger definition for SLF4J
Defined logger explicitly with LoggerFactory.getLogger(...)
Ensured compatibility with Lombok's @slf4j annotation
Fixed compilation error caused by missing 'log' variable

Files Processed (2)

leader-followers/pom.xml (1 hunk)
leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java (2 hunks)

Actionable Comments (0)

Skipped Comments (1)

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java [26-36]

enhancement: "Improve logging in finally block."

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

a758249: Merge branch 'iluwatar:master' into master

Files Processed (0)

Actionable Comments (0)

Skipped Comments (0)

@slf4j

- Defined logger explicitly with LoggerFactory.getLogger(...) - Ensured compatibility with Lombok's @slf4j annotation - Fixed compilation error caused by missing 'log' variable

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

a98e1ea: fix: add missing logger definition for SLF4J
Defined logger explicitly with LoggerFactory.getLogger(...)
Ensured compatibility with Lombok's @slf4j annotation
Fixed compilation error caused by missing 'log' variable

Files Processed (2)

leader-followers/pom.xml (1 hunk)
leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java (2 hunks)

Actionable Comments (0)

Skipped Comments (1)

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java [26-36]

enhancement: "Improve exception handling and logging in finally block."

2897robo · 2025-04-08T02:28:51Z

Hi @iluwatar, thank you for the feedback!

✅ I've addressed your review comment by defining the SLF4J logger explicitly instead of using Lombok's @Slf4j.
✅ Also confirmed the proper use of try-finally since ExecutorService is not AutoCloseable.

Let me know if there's anything else I should improve 🙌

iluwatar · 2025-04-08T17:13:36Z

leader-followers/pom.xml

+    <dependency>
+      <groupId>org.projectlombok</groupId>
+      <artifactId>lombok</artifactId>
+      <version>1.18.24</version>
+      <scope>provided</scope>
+    </dependency>


The dependency already exists in parent pom.xml, so no need to declare it here

iluwatar · 2025-04-08T17:17:17Z

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java

- */
+@Slf4j
 public class App {
+  private static final Logger log = LoggerFactory.getLogger(App.class);


No need to declare the logger like this. After inserting the annotation you can just log like LOGGER.info("hello")

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

4ef609c: fix: add missing logger definition for SLF4J

Files Processed (1)

leader-followers/src/main/java/com/iluwatar/leaderfollowers/App.java (2 hunks)

Actionable Comments (0)

Skipped Comments (0)

sonarqubecloud · 2025-04-09T05:32:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

2897robo · 2025-04-09T05:34:33Z

✅ Updated code based on review:

Removed duplicate Lombok dependency (exists in parent POM)
Removed manual logger definition
Using Lombok's @slf4j annotation with LOGGER.warn() syntax

Let me know if there's anything else to revise.
Thanks for reviewing as always! 🙌

2897robo · 2025-04-10T05:07:05Z

Hi @iluwatar! 🙌
I’ve updated the PR based on all your feedback:

✅ Removed the redundant Lombok dependency (parent POM already includes it)
✅ Removed manual logger definition
✅ Replaced it with Lombok's @slf4j and updated to LOGGER.warn(...) style

All checks have passed ✅ and there are no conflicts with base branch.
When you have a moment, I’d really appreciate a final review or merge.
Thanks again for your time and guidance! 🙏

iluwatar · 2025-04-10T16:34:16Z

Looks good! Thank you for the contribution 🎉

@all-contributors please add @2897robo for code

allcontributors · 2025-04-10T16:34:19Z

@iluwatar

@2897robo already contributed before to code

2897robo · 2025-04-10T16:48:57Z

Thank you so much for reviewing and merging! 🙏
It was a valuable learning experience to contribute and receive feedback.
I’ll continue contributing whenever I can!

Much appreciated! 🚀

2897robo added 2 commits April 7, 2025 13:20

Merge branch 'iluwatar:master' into master

c217d2d

github-actions bot reviewed Apr 7, 2025

View reviewed changes

iluwatar requested changes Apr 7, 2025

View reviewed changes

iluwatar added the status: under review label Apr 7, 2025

github-actions bot reviewed Apr 8, 2025

View reviewed changes

2897robo force-pushed the master branch from a758249 to 0f78c56 Compare April 8, 2025 02:01

fix: add missing logger definition for SLF4J

a98e1ea

- Defined logger explicitly with LoggerFactory.getLogger(...) - Ensured compatibility with Lombok's @slf4j annotation - Fixed compilation error caused by missing 'log' variable

2897robo force-pushed the master branch from 0f78c56 to a98e1ea Compare April 8, 2025 02:12

github-actions bot reviewed Apr 8, 2025

View reviewed changes

iluwatar requested changes Apr 8, 2025

View reviewed changes

fix: add missing logger definition for SLF4J

4ef609c

github-actions bot reviewed Apr 9, 2025

View reviewed changes

iluwatar approved these changes Apr 10, 2025

View reviewed changes

iluwatar merged commit 6cadf25 into iluwatar:master Apr 10, 2025
2 checks passed

Uh oh!

fix: handle awaitTermination result and ensure proper ExecutorService shutdown #3244

fix: handle awaitTermination result and ensure proper ExecutorService shutdown #3244

Uh oh!

Conversation

2897robo commented Apr 7, 2025

What does this PR do?

What was the problem?

What does this PR change?

Why not try-with-resources?

References

Uh oh!

github-actions bot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Changes

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

2897robo commented Apr 7, 2025

Uh oh!

iluwatar Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

2897robo commented Apr 8, 2025

Uh oh!

iluwatar Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

iluwatar Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

sonarqubecloud bot commented Apr 9, 2025

Quality Gate passed

Uh oh!

2897robo commented Apr 9, 2025

Uh oh!

2897robo commented Apr 10, 2025

Uh oh!

Uh oh!

iluwatar commented Apr 10, 2025

Uh oh!

allcontributors bot commented Apr 10, 2025

Uh oh!

2897robo commented Apr 10, 2025

Uh oh!

Uh oh!

github-actions bot commented Apr 7, 2025 •

edited

Loading