(chores) Fix SonarCloud S2095: close HttpClient resources properly#24358
Draft
gnodet wants to merge 1 commit into
Draft
(chores) Fix SonarCloud S2095: close HttpClient resources properly#24358gnodet wants to merge 1 commit into
gnodet wants to merge 1 commit into
Conversation
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 568 tested, 29 compile-only — current: 76 all testedMaveniverse Scalpel detected 597 affected modules (current approach: 76).
|
davsclaus
reviewed
Jul 1, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
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") |
Contributor
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?
Contributor
Author
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") |
Contributor
There was a problem hiding this comment.
Same as AggregateReifier — this was narrowed to statement level in #22343 after explicit review feedback.
Contributor
Author
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") |
Contributor
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?
Contributor
Author
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.
gnodet
added a commit
to gnodet/camel
that referenced
this pull request
Jul 1, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
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.
Comment on lines
120
to
124
| // 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<>(); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Fixed in 32c5573 — updated comments to remove misleading "GC'd" rationale, added try-with-resources for LineNumberReader, and corrected ShellPanel comment to reference virtualTerminal ownership.
Comment on lines
163
to
167
| // 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<>(); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Fixed in 32c5573 — updated comments to remove misleading "GC'd" rationale, added try-with-resources for LineNumberReader, and corrected ShellPanel comment to reference virtualTerminal ownership.
Comment on lines
55
to
57
| // 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 { |
Contributor
Author
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Fixed in 32c5573 — updated comments to remove misleading "GC'd" rationale, added try-with-resources for LineNumberReader, and corrected ShellPanel comment to reference virtualTerminal ownership.
Comment on lines
394
to
396
| // DelegateOutputStream and feedbackOutput are owned by the virtualTerminal; closed in stopShell() | ||
| @SuppressWarnings("java:S2095") | ||
| private void startShell(int width, int height) { |
Contributor
Author
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Fixed in 32c5573 — updated comments to remove misleading "GC'd" rationale, added try-with-resources for LineNumberReader, and corrected ShellPanel comment to reference virtualTerminal ownership.
gnodet
added a commit
to gnodet/camel
that referenced
this pull request
Jul 2, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
changed the title
davsclaus
approved these changes
Jul 2, 2026
oscerd
reviewed
Jul 2, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
The resource-leak changes look correct — I went through each closure and found no premature-close of a shared or returned resource. In particular, the return hc.httpClient.send(…, ofByteArray()) inside try-with-resources in QdrantInfraService.put is safe because the body is fully buffered to byte[] before the client closes (it would only be a bug with a streaming ofInputStream handler). The @SuppressWarnings cases are genuine false positives, there's no public-API change, and the CloseableHttpClient wrapper is a no-op on Java 17 and a real close() on 21+.
Main thing: build (17) is currently red. It's a forked-JVM crash in camel-jbang-core mvn test (the last test to start was CamelReloadActionTest, which spawns subprocesses) with no assertion failure in the surefire report; build (25) was cancelled by fail-fast rather than failing on its own. Since this PR's only camel-jbang-core changes are @SuppressWarnings annotations (CatalogLoader, PluginHelper) — behaviorally inert — this looks like a flaky/infra crash rather than something the PR introduced. Could you re-run the failed job? Just can't merge while it's red.
Minor (non-blocking):
- The 34-line
CloseableHttpClientwrapper is duplicated three times (camel-test-infra-openai-mock,camel-test-infra-qdrant, and a private inner class inUpdateCamelReleasesMojo) — a little ironic in a Sonar-cleanup PR since SonarCloud flags duplication, though understandable given test-infra modules deliberately don't cross-depend. - The commit messages are free-form; the documented convention for Sonar sweeps is
(chores): fix SonarCloud <rule> in <component>.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet
force-pushed
the
sonar/s2095-fix-resource-leaks
branch
from
7b3383a to
b8cf631
Compare
gnodet
changed the title
Wrap java.net.http.HttpClient in an AutoCloseable adapter and use try-with-resources in OpenAI mock tests, Qdrant infra service, and UpdateCamelReleasesMojo. This resolves S2095 resource leak warnings without changing behavior on JDK 17 (where HttpClient.close() does not exist) while correctly closing on JDK 21+. Also remove @SuppressWarnings("java:S2095") annotations from reifiers, CatalogLoader, PluginHelper, ShellPanel, and SalesforceComponent — those were false positives now marked directly in SonarCloud. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
force-pushed
the
sonar/s2095-fix-resource-leaks
branch
from
b8cf631 to
f8bc1e6
Compare
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
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.
Summary
Wrap
java.net.http.HttpClientin anAutoCloseableadapter and usetry-with-resourcesto resolve SonarCloud S2095 resource leak warnings:camel-test-infra-openai-mock: Wrap allHttpClientusages in tests withCloseableHttpClientadapter andtry-with-resourcescamel-test-infra-qdrant: Same pattern forQdrantInfraServiceUpdateCamelReleasesMojo: Same pattern for Maven plugin HTTP calls@SuppressWarnings("java:S2095")— these were false positives now marked directly in SonarCloudThe
CloseableHttpClientadapter is a no-op on JDK 17 (whereHttpClient.close()doesn't exist) and callsclose()properly on JDK 21+.Test plan
CloseableHttpClientadapter verified to work on both JDK 17 and JDK 21+Claude Code on behalf of Guillaume Nodet