Fix S2095 resource leak warnings across multiple modules#24358
Conversation
Suppress SonarCloud S2095 (Resources should be closed) false positives where resource lifecycle is intentionally managed elsewhere: - SalesforceComponent: ExecutorService managed by SalesforceHttpClient - AggregateReifier/ThreadsReifier: ExecutorService managed via shutdownThreadPool flag - CatalogLoader/PluginHelper: URLClassLoader kept open as plugin classloader - ShellPanel: streams owned by virtualTerminal, closed in stopShell() - QdrantInfraService/UpdateCamelReleasesMojo: HttpClient not AutoCloseable before Java 21 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
💡 Manual integration tests recommended:
Build reactor — dependencies compiled but only changed modules were tested (6 modules)
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Review: Fix S2095 resource leak warnings across multiple modules
Reverts prior review feedback without explanation (AggregateReifier, ThreadsReifier, SalesforceComponent)
The prior PR #22343 (CAMEL-23271), also by Guillaume, went through four review rounds specifically to narrow suppression scope. Reviewer @squakez explicitly asked to move @SuppressWarnings from method level to statement level to avoid masking future real leaks. The final version used:
- Statement-level
@SuppressWarningson local variable declarations (AggregateReifier, ThreadsReifier) - Inline
// NOSONARcomment (SalesforceComponent)
This PR moves all three back to method-level @SuppressWarnings, which is the exact approach the reviewer rejected. The PR description acknowledges the move but does not explain why the narrower scope was insufficient. If SonarCloud doesn't recognize statement-level annotations or // NOSONAR in some scanner configuration, that would justify the change — but it should be stated.
New suppressions look good
The 5 new suppressions (CatalogLoader, PluginHelper, ShellPanel, QdrantInfraService, UpdateCamelReleasesMojo) are well-reasoned with accurate comments explaining the lifecycle ownership.
Questions
- Is SonarCloud still flagging the three files that already had suppressions from CAMEL-23271? If so, the move to method-level is justified as a workaround, but worth noting in the PR description.
- The PR has no linked JIRA ticket. Per project conventions, SonarCloud fixes typically use commit format
(chores): fix SonarCloud <rule> in <component>— minor style point.
This review does not replace specialized tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
|
|
||
| // ExecutorService lifecycle is managed by AggregateProcessor via shutdownThreadPool flag | ||
| @SuppressWarnings("java:S2095") |
There was a problem hiding this comment.
This @SuppressWarnings was intentionally placed at the local variable declaration level in PR #22343 after reviewer feedback from @squakez asked to narrow the scope to avoid masking future real leaks in the same method. Moving it back to method level reverses that decision.
If SonarCloud is not recognizing the statement-level annotation, that would justify this change — could you confirm?
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Yes — SonarCloud is still flagging this file despite the statement-level @SuppressWarnings from PR #22343. The annotation sits on line 80 (the variable declaration), but SonarCloud tracks the resource flow to a different exit point and flags line 86 instead, bypassing the annotation.
Confirmed via API: https://sonarcloud.io/api/issues/search?componentKeys=apache_camel&branch=main&rules=java:S2095 still lists AggregateReifier.java:86 as an open issue.
Moving to method-level is the only approach SonarCloud recognizes for this pattern. We acknowledge it broadens the scope, but given that this method only creates one ExecutorService with well-documented lifecycle, the risk of masking future leaks is low.
| } | ||
|
|
||
| // ExecutorService lifecycle is managed by ThreadsProcessor via shutdownThreadPool flag | ||
| @SuppressWarnings("java:S2095") |
There was a problem hiding this comment.
Same as AggregateReifier — this was narrowed to statement level in #22343 after explicit review feedback.
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Same situation — SonarCloud still flags ThreadsReifier.java:75 despite the statement-level @SuppressWarnings on line 47. The annotation doesn't cover the line SonarCloud is tracking the resource flow to.
| } | ||
|
|
||
| // ExecutorService lifecycle is managed by SalesforceHttpClient | ||
| @SuppressWarnings("java:S2095") |
There was a problem hiding this comment.
The // NOSONAR inline comment was the accepted approach from PR #22343 review. Replacing it with method-level @SuppressWarnings widens the suppression scope. Same question: is SonarCloud not picking up the // NOSONAR?
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Confirmed — SonarCloud still flags SalesforceComponent.java:968 despite the // NOSONAR on line 967. The NOSONAR comment is on the constructor call line, but SonarCloud flags the next line (the argument). Method-level @SuppressWarnings is the fallback that SonarCloud actually respects for this pattern.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses SonarCloud rule S2095 (Resources should be closed) warnings across several Camel modules by replacing inline suppressions with annotated @SuppressWarnings("java:S2095") plus explanatory comments, in cases where resource lifecycles are intentionally managed elsewhere (or the type is not AutoCloseable on the project’s target JDK).
Changes:
- Add/move
@SuppressWarnings("java:S2095")to method scope (or other scope) with rationale comments for flagged “resource leak” patterns. - Clarify intentional ownership/lifecycle management for executors, classloaders, and output streams.
- Suppress S2095 for
java.net.http.HttpClientusage where the type is notAutoCloseableon the target Java version.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/maven/camel-package-maven-plugin/src/main/java/org/apache/camel/maven/packaging/UpdateCamelReleasesMojo.java | Adds S2095 suppressions for JDK HttpClient usage in release-fetching helpers. |
| test-infra/camel-test-infra-qdrant/src/main/java/org/apache/camel/test/infra/qdrant/services/QdrantInfraService.java | Adds S2095 suppression for HttpClient.newHttpClient() in the default put() helper. |
| dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/ShellPanel.java | Suppresses S2095 for terminal-related streams owned by the virtual terminal lifecycle. |
| dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PluginHelper.java | Suppresses S2095 for a URLClassLoader intentionally retained as a plugin classloader. |
| dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/CatalogLoader.java | Suppresses S2095 for a classloader intentionally retained by the version manager. |
| core/camel-core-reifier/src/main/java/org/apache/camel/reifier/ThreadsReifier.java | Moves S2095 suppression to method level for executor lifecycle managed by ThreadsProcessor. |
| core/camel-core-reifier/src/main/java/org/apache/camel/reifier/AggregateReifier.java | Moves S2095 suppression to method level for executor lifecycle managed by AggregateProcessor. |
| components/camel-salesforce/camel-salesforce-component/src/main/java/org/apache/camel/component/salesforce/SalesforceComponent.java | Replaces inline NOSONAR with method-level S2095 suppression for executor lifecycle managed by SalesforceHttpClient. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // HttpClient does not implement AutoCloseable before Java 21; short-lived and GC'd | ||
| @SuppressWarnings("java:S2095") | ||
| private List<ReleaseModel> processReleases(List<String> urls) throws Exception { | ||
| List<ReleaseModel> answer = new ArrayList<>(); | ||
|
|
| // HttpClient does not implement AutoCloseable before Java 21; short-lived and GC'd | ||
| @SuppressWarnings("java:S2095") | ||
| private List<String> fetchCamelReleaseLinks(String gitUrl) throws Exception { | ||
| List<String> answer = new ArrayList<>(); | ||
|
|
| // HttpClient does not implement AutoCloseable before Java 21; short-lived and GC'd | ||
| @SuppressWarnings("java:S2095") | ||
| default HttpResponse<byte[]> put(String path, Map<Object, Object> body) throws Exception { |
| // DelegateOutputStream and feedbackOutput are owned by the virtualTerminal; closed in stopShell() | ||
| @SuppressWarnings("java:S2095") | ||
| private void startShell(int width, int height) { |
Summary
Claude Code on behalf of Guillaume Nodet
Suppress SonarCloud S2095 (Resources should be closed) false positives across 8 files in 6 modules where the resource lifecycle is intentionally managed by an external owner or the resource is not AutoCloseable in the target Java version.
Existing suppressions moved to method level (SonarCloud workaround)
Three files already had statement-level
@SuppressWarnings("java:S2095")or// NOSONARfrom PR #22343 (CAMEL-23271), but SonarCloud still flags them because it tracks the resource flow to a different exit line than where the annotation sits:SalesforceComponent.createHttpClient():// NOSONARon line 967, SonarCloud flags line 968AggregateReifier.createAggregator():@SuppressWarningson line 80 (variable decl), SonarCloud flags line 86ThreadsReifier.createProcessor():@SuppressWarningson line 47, SonarCloud flags line 75Moving to method-level
@SuppressWarningsis the only placement SonarCloud recognizes for these resource-flow patterns. We acknowledge this broadens the suppression scope from the PR #22343 approach, but each method only creates one managedExecutorService, so the risk of masking future real leaks is low.New suppressions
URLClassLoaderinCatalogLoader.loadQuarkusCatalog()andPluginHelper.loadFromCache()is intentionally kept open as the plugin/version-manager classloaderDelegateOutputStreaminShellPanel.startShell()is owned by the virtual terminal and closed instopShell()HttpClient.newHttpClient()inQdrantInfraService.put()—java.net.http.HttpClientdoes not implementAutoCloseablebefore Java 21HttpClient.newHttpClient()inUpdateCamelReleasesMojo— same Java 21 limitationAll suppressions include a comment explaining why the resource does not need closing at the flagged location.
Test plan
mvn formatter:format impsort:sortpasses on all affected modulesmvn clean install -Dquicklypasses on all affected modules🤖 Generated with Claude Code