Skip to content

Conversation

@minwoox
Copy link
Contributor

@minwoox minwoox commented Jan 7, 2026

Found this from the CI:
Leak is detected in com.linecorp.armeria.server.HttpServerConnectMethodTest.connectMethodDisallowedInHttp1()

Found this from the CI:
Leak is detected in com.linecorp.armeria.server.HttpServerConnectMethodTest.connectMethodDisallowedInHttp1()
@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

A single test file is refactored to use try-with-resources for automatic ClientFactory closure instead of explicit closeAsync() call, with WebClient construction moved inside the block. No functional behavior changes.

Changes

Cohort / File(s) Summary
Resource Management Refactoring
core/src/test/java/com/linecorp/armeria/server/HttpServerConnectMethodTest.java
Wrapped ClientFactory initialization in try-with-resources block for automatic resource closure; moved WebClient construction into the block; removed explicit factory.closeAsync() call. Scope and test logic remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A test that once did close with care,
Now wraps its resources with flair,
Try-with-resources, so neat and clean,
The finest cleanup ever seen! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the specific fix - addressing a resource leak in HttpServerConnectMethodTest, matching the changeset's focus on wrapping ClientFactory in try-with-resources.
Description check ✅ Passed The description directly relates to the changeset by explaining the source of the fix (a leak detected in the test) and the intended resolution.

✏️ Tip: You can configure your own custom Pre-merge Checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 173788c and a668def.

📒 Files selected for processing (1)
  • core/src/test/java/com/linecorp/armeria/server/HttpServerConnectMethodTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java

⚙️ CodeRabbit configuration file

**/*.java: - The primary coding conventions and style guide for this project are defined in site/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.

2. Specific check for @UnstableApi

  • Review all newly added public classes and methods to ensure they have the @UnstableApi annotation.
  • However, this annotation is NOT required under the following conditions:
    • If the class or method is located in a package containing .internal or .testing.
    • If the class or method is located in a test source set.
    • If a public method is part of a class that is already annotated with @UnstableApi.

Files:

  • core/src/test/java/com/linecorp/armeria/server/HttpServerConnectMethodTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
  • GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
  • GitHub Check: build-ubicloud-standard-16-jdk-25
  • GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
  • GitHub Check: build-ubicloud-standard-16-jdk-11
  • GitHub Check: build-ubicloud-standard-16-jdk-17-leak
  • GitHub Check: build-macos-latest-jdk-25
  • GitHub Check: build-ubicloud-standard-16-jdk-8
  • GitHub Check: site
  • GitHub Check: lint
  • GitHub Check: flaky-tests
  • GitHub Check: Kubernetes Chaos test
  • GitHub Check: Summary
🔇 Additional comments (2)
core/src/test/java/com/linecorp/armeria/server/HttpServerConnectMethodTest.java (2)

69-74: Excellent fix for the resource leak.

The try-with-resources pattern properly ensures the ClientFactory is automatically closed after the test completes, which directly addresses the resource leak mentioned in the PR objectives. Moving the WebClient construction inside the block is correct since it depends on the factory's lifecycle.


76-101: LGTM - Test logic properly scoped.

The test logic remains functionally identical with appropriate indentation for the new try-with-resources scope. All client usage correctly occurs within the factory's lifecycle, and the connection pool assertions validate that resources are being properly managed.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.38%. Comparing base (8150425) to head (464a7f0).
⚠️ Report is 315 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6583      +/-   ##
============================================
- Coverage     74.46%   74.38%   -0.08%     
- Complexity    22234    23715    +1481     
============================================
  Files          1963     2128     +165     
  Lines         82437    88594    +6157     
  Branches      10764    11585     +821     
============================================
+ Hits          61385    65900    +4515     
- Misses        15918    17161    +1243     
- Partials       5134     5533     +399     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@minwoox minwoox merged commit 84ecc9a into line:main Jan 7, 2026
16 of 18 checks passed
@minwoox minwoox deleted the fix_leak branch January 7, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants