fix: Unwrap DelegatingEventLoopGroup in isSupported() and supportsDomainSockets()#6633
fix: Unwrap DelegatingEventLoopGroup in isSupported() and supportsDomainSockets()#6633
Conversation
…ainSockets() The find() method already unwraps DelegatingEventLoopGroup, but isSupported() and supportsDomainSockets() bypass find() and call findOrNull() directly. Without unwrapping, passing a wrapped group (e.g. ShutdownConfigurableEventLoopGroup) to these methods would incorrectly return false.
novoj
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
📝 WalkthroughWalkthroughUnwraps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes an IllegalStateException that occurs when using a ShutdownConfigurableEventLoopGroup (a wrapped EventLoopGroup) with Armeria's ClientFactory. The issue arose from the newly added graceful shutdown configuration feature in version 1.36.0.
Changes:
- Added DelegatingEventLoopGroup unwrapping to TransportType.find() to fix the primary issue
- Added DelegatingEventLoopGroup unwrapping to TransportType.isSupported() for consistency
- Added DelegatingEventLoopGroup unwrapping to TransportType.supportsDomainSockets() for consistency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/util/TransportType.java (1)
107-107: Simplify ternary to&&♻️ Proposed simplification
- return found != null ? found.supportsDomainSockets() : false; + return found != null && found.supportsDomainSockets();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/common/util/TransportType.java` at line 107, The ternary expression returning found != null ? found.supportsDomainSockets() : false should be simplified to use short-circuit AND; update the return in TransportType (the method referencing the local variable found and calling supportsDomainSockets()) to return found != null && found.supportsDomainSockets() to make the intent clearer and remove the redundant conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/linecorp/armeria/common/util/TransportType.java`:
- Around line 100-102: TransportType contains three identical single-layer
unwraps of DelegatingEventLoopGroup (the blocks around lines where
eventLoopGroup instanceof DelegatingEventLoopGroup is checked); implement a
private helper method (e.g., unwrapDelegatingEventLoopGroup(EventLoopGroup))
that uses a while loop to repeatedly call ((DelegatingEventLoopGroup)
g).delegate() until the returned EventLoopGroup is not a
DelegatingEventLoopGroup, then replace the three identical blocks with a call to
this helper so supportsDomainSockets(), isSupported(), and find() operate on the
fully-unwrapped EventLoopGroup.
---
Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/common/util/TransportType.java`:
- Line 107: The ternary expression returning found != null ?
found.supportsDomainSockets() : false should be simplified to use short-circuit
AND; update the return in TransportType (the method referencing the local
variable found and calling supportsDomainSockets()) to return found != null &&
found.supportsDomainSockets() to make the intent clearer and remove the
redundant conditional.
| if (eventLoopGroup instanceof DelegatingEventLoopGroup) { | ||
| eventLoopGroup = ((DelegatingEventLoopGroup) eventLoopGroup).delegate(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | xargs grep -l "interface DelegatingEventLoopGroup\|class DelegatingEventLoopGroup" | head -5Repository: line/armeria
Length of output: 139
🏁 Script executed:
rg -A10 "interface DelegatingEventLoopGroup" -t javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
rg -A5 "delegate()" -t java | head -100Repository: line/armeria
Length of output: 11960
🏁 Script executed:
# Check the three specific locations mentioned
cat -n core/src/main/java/com/linecorp/armeria/common/util/TransportType.java | sed -n '98,105p; 187,195p; 205,212p'Repository: line/armeria
Length of output: 1463
🏁 Script executed:
cat -n core/src/main/java/com/linecorp/armeria/common/util/DelegatingEventLoopGroup.javaRepository: line/armeria
Length of output: 7269
🏁 Script executed:
rg "implements DelegatingEventLoopGroup|extends.*DelegatingEventLoopGroup" -t java -B2 -A10Repository: line/armeria
Length of output: 1776
🏁 Script executed:
rg "class.*DelegatingEventLoopGroup|interface.*DelegatingEventLoopGroup" -t java -B2 -A15Repository: line/armeria
Length of output: 4465
Extract unwrap logic into a while-loop helper to handle nested DelegatingEventLoopGroup wrappers and eliminate code duplication
The three identical unwrap blocks at lines 100–102, 189–191, and 207–209 only peel one layer. Since DelegatingEventLoopGroup.delegate() returns a generic EventLoopGroup (which could itself be another DelegatingEventLoopGroup), nested wrapping is possible. A single-level unwrap will leave the inner wrapper undetected, causing supportsDomainSockets() and isSupported() to return false incorrectly, and find() to throw a spurious exception.
A private helper with a while loop resolves both the robustness issue and the DRY violation:
♻️ Proposed refactor
+ private static EventLoopGroup unwrapDelegate(EventLoopGroup eventLoopGroup) {
+ while (eventLoopGroup instanceof DelegatingEventLoopGroup) {
+ eventLoopGroup = ((DelegatingEventLoopGroup) eventLoopGroup).delegate();
+ }
+ return eventLoopGroup;
+ }Then replace all three blocks:
public static boolean supportsDomainSockets(EventLoopGroup eventLoopGroup) {
requireNonNull(eventLoopGroup, "eventLoopGroup");
- if (eventLoopGroup instanceof DelegatingEventLoopGroup) {
- eventLoopGroup = ((DelegatingEventLoopGroup) eventLoopGroup).delegate();
- }
+ eventLoopGroup = unwrapDelegate(eventLoopGroup);
...
}
public static boolean isSupported(EventLoopGroup eventLoopGroup) {
requireNonNull(eventLoopGroup, "eventLoopGroup");
- if (eventLoopGroup instanceof DelegatingEventLoopGroup) {
- eventLoopGroup = ((DelegatingEventLoopGroup) eventLoopGroup).delegate();
- }
+ eventLoopGroup = unwrapDelegate(eventLoopGroup);
...
}
private static TransportType find(EventLoopGroup eventLoopGroup) {
- if (eventLoopGroup instanceof DelegatingEventLoopGroup) {
- eventLoopGroup = ((DelegatingEventLoopGroup) eventLoopGroup).delegate();
- }
+ eventLoopGroup = unwrapDelegate(eventLoopGroup);
...
}Also applies to: 189–191, 207–209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/com/linecorp/armeria/common/util/TransportType.java`
around lines 100 - 102, TransportType contains three identical single-layer
unwraps of DelegatingEventLoopGroup (the blocks around lines where
eventLoopGroup instanceof DelegatingEventLoopGroup is checked); implement a
private helper method (e.g., unwrapDelegatingEventLoopGroup(EventLoopGroup))
that uses a while loop to repeatedly call ((DelegatingEventLoopGroup)
g).delegate() until the returned EventLoopGroup is not a
DelegatingEventLoopGroup, then replace the three identical blocks with a call to
this helper so supportsDomainSockets(), isSupported(), and find() operate on the
fully-unwrapped EventLoopGroup.
…ainSockets() The find() method already unwraps DelegatingEventLoopGroup, but isSupported() and supportsDomainSockets() bypass find() and call findOrNull() directly. Without unwrapping, passing a wrapped group (e.g. ShutdownConfigurableEventLoopGroup) to these methods would incorrectly return false.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/client/EventLoopGroupBuilderIntegrationTest.java (1)
46-54: Consider guardingeventLoopGroupagainst leaks ifClientFactoryconstruction throws.
eventLoopGroupis created outside thetryblock, but theClientFactory(which owns its shutdown viaworkerGroup(eventLoopGroup, true)) is only entered inside thetry. IfClientFactory.builder().build()throws, theeventLoopGroupthreads are never terminated. In test code with zero-duration graceful shutdown this is largely harmless, but a nested try-with-resources (or moving the group into its owntry) makes the intent explicit and protects against flaky teardown if a future change introduces a non-zero duration.♻️ Suggested fix: guard `eventLoopGroup` with its own try-with-resources
- final EventLoopGroup eventLoopGroup = EventLoopGroups - .builder() - .numThreads(4) - .gracefulShutdown(Duration.ofMillis(0), Duration.ofMillis(0)) - .build(); - - try (ClientFactory clientFactory = ClientFactory.builder() - .workerGroup(eventLoopGroup, true) - .build()) { + try (ClientFactory clientFactory = ClientFactory.builder() + .workerGroup(EventLoopGroups.builder() + .numThreads(4) + .gracefulShutdown(Duration.ofMillis(0), Duration.ofMillis(0)) + .build(), true) + .build()) {This keeps the
eventLoopGrouplifetime entirely managed by theClientFactory(viashutdownOnClose = true), so a singletryis sufficient and there is no window for a leak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/linecorp/armeria/client/EventLoopGroupBuilderIntegrationTest.java` around lines 46 - 54, Create the EventLoopGroup inside its own try-with-resources so it is closed if ClientFactory.builder().build() throws: instantiate EventLoopGroup via EventLoopGroups.builder().numThreads(4).gracefulShutdown(...).build() in a try(...) resource, then inside that try create the ClientFactory with ClientFactory.builder().workerGroup(eventLoopGroup, true).build() (nested try-with-resources) so workerGroup(..., true) still sets shutdownOnClose but the EventLoopGroup is guarded against leaks on builder failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/test/java/com/linecorp/armeria/client/EventLoopGroupBuilderIntegrationTest.java`:
- Around line 46-54: Create the EventLoopGroup inside its own try-with-resources
so it is closed if ClientFactory.builder().build() throws: instantiate
EventLoopGroup via
EventLoopGroups.builder().numThreads(4).gracefulShutdown(...).build() in a
try(...) resource, then inside that try create the ClientFactory with
ClientFactory.builder().workerGroup(eventLoopGroup, true).build() (nested
try-with-resources) so workerGroup(..., true) still sets shutdownOnClose but the
EventLoopGroup is guarded against leaks on builder failure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6633 +/- ##
============================================
- Coverage 74.46% 74.26% -0.20%
- Complexity 22234 23936 +1702
============================================
Files 1963 2160 +197
Lines 82437 89494 +7057
Branches 10764 11702 +938
============================================
+ Hits 61385 66466 +5081
- Misses 15918 17442 +1524
- Partials 5134 5586 +452 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Question) Would it be possible to instead modify so that ShutdownConfigurableEventLoopGroup extends from IoEventLoopGroup?
This way, we don't need to worry about the unwrapping logic from usage locations.
It seems like TransportTypeProvider only returns an MultiThreadIoEventLoopGroup type at the moment, so it seems safe to assume only an IoEventLoopGroup will be returned.
i.e.
class DelegatingEventLoopGroup implements IoEventLoopGroup {
...and
...
final EventLoopGroup eventLoopGroup = type.newEventLoopGroup(numThreads, unused -> factory);
// Wrap with shutdown configuration if non-default values are used
if (shutdownQuietPeriodMillis != DEFAULT_SHUTDOWN_QUIET_PERIOD_MILLIS ||
shutdownTimeoutMillis != DEFAULT_SHUTDOWN_TIMEOUT_MILLIS) {
checkArgument(eventLoopGroup instanceof IoEventLoopGroup,
"Expected a type of IOEventLoopGroup, but got: %s", eventLoopGroup.getClass());
final IoEventLoopGroup ioEventLoopGroup = (IoEventLoopGroup) eventLoopGroup;
return new ShutdownConfigurableEventLoopGroup(
ioEventLoopGroup, shutdownQuietPeriodMillis, shutdownTimeoutMillis);
...
The find() method already unwraps DelegatingEventLoopGroup, but isSupported() and supportsDomainSockets() bypass find() and call findOrNull() directly. Without unwrapping, passing a wrapped group (e.g. ShutdownConfigurableEventLoopGroup) to these methods would incorrectly return false.
Motivation:
In lastest version of armeria (
1.36.0) new functionality allowed to configure shutdown on client (excerpt from release notes):Unfortunatelly this throws exception:
Modifications:
isSupported() and supportsDomainSockets() bypass find() and call findOrNull() directly. Without unwrapping, passing a wrapped group (e.g. ShutdownConfigurableEventLoopGroup) to these methods would incorrectly return false.
Result: