Fix a bug where a connection may be not reused when using `RetryingClient` #5290

ikhoon · 2023-11-07T14:59:37Z

Motivation:

Armeria's HttpChannelPool is bound to an EventLoop. Different EventLoops have different HttpChannelPools. In other words, in order to reuse a connection for an Endpoint, the same EventLoop must be selected.

When creating a derived client in RetryingClient, a new endpoint is selected for each retry, but since the EventLoop of the parent is used as is. That causes the Endpoint can't use the existing connection pool for multiplexing and makes a new connection.

Modifications:

Use EventLoopScheduler to return constant EventLoops for the same endpoint.
Allow setting EndpointGroup in ClientRequestContextBuilder for testability.

Result:

You no longer see a connection leak when using RetryingClient with EndpointGroup.

…client. Motivation: Armeria's `HttpChannelPool` is bound to an `EventLoop`. Different `EventLoop`s have different `HttpChannelPool`s. In other words, in order to reuse a connection for an `Endpoint`, the same `EventLoop` must be selected. When creating a derived client in `RetryingClient`, a new endpoint is selected for each retry, but since the `EventLoop` of parent is used as is. That causes that the `Endpoint` can't use the existing connection pool for multiplexing and makes a new connection created. Modifications: - Use `EventLoopScheduler` to return constant `EventLoop`s for the same endpoint. - Allow setting `EndpointGroup` in `ClientRequestContextBuilder` for testability. Result: - You no longer not see a connection leak when using `RetryingClient` with `EndpointGroup`.

github-actions · 2023-11-07T15:39:15Z

🔍 Build Scan® (commit: `d73eabc`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/yqz43pyaliwwe
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/y6zy5h7gu3mvk
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/mtim4c3qfnjpk
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/y3zq7rsgphmpo
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/s4dwc4wcq42he
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/ab7dwbr53ngba
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/hzn7ykjbipkps
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/jv5fxqm7bjine

minwoox · 2023-11-08T01:24:21Z

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

+            acquireEventLoop(endpoint);
+        }
+
+        log = RequestLog.builder(this);


It seems like we should do this before calling acquireEventLoop

jrhee17

Left some minor questions 🙇

jrhee17 · 2023-11-08T01:23:09Z

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

@@ -531,6 +528,14 @@ private DefaultClientRequestContext(DefaultClientRequestContext ctx,

        this.endpointGroup = endpointGroup;
        updateEndpoint(endpoint);
+        if (ctx.endpoint() == endpoint || endpoint == null) {


Question) Is this compare by reference as opposed to equality intentionally?

Originally, reference comparison was used to know the derivation was the initial attempt. The logic is changed to check the initial attempt more explicitly.

reference comparison was used to know the derivation was the initial attempt

Are you suggesting that ctx.endpoint() == endpoint implies that it is the initial attempt?

Anyways, I was actually implying that it is possible that an endpoint group updates its endpoints, and comparing by reference won't reflect it. I think it's fine if you are aware of this

ctx.log.children().isEmpty() would be enough but added reference equality for double check.
ctx.endpoint() == endpoint && ctx.log.children().isEmpty()

If a derivation is not an initial attempt, acquireEventloop() needs to be called even for the same Endpoint to update the internal state of DefaultEventLoopScheduler.

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

core/src/main/java/com/linecorp/armeria/client/ClientRequestContextBuilder.java

...rc/test/java/com/linecorp/armeria/internal/client/DerivedClientRequestContextClientTest.java

trustin · 2023-11-08T02:29:53Z

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

+        if (ctx.endpoint() == endpoint || endpoint == null) {
+            eventLoop = ctx.eventLoop().withoutContext();
+        } else {
+            acquireEventLoop(endpoint);
+        }


What happens if we just always call acquireEventLoop()? (maybe except when endpoint == null)

It sounds like a good idea to update the lastActivityTimeNanos.

However if we always call acquireEventLoop(), the number of active requests may be increased for the initial attempt. Let me add an exception for the initial attempt.

Also, when ctx.endpoint() == endpoint and the maxNumEventLoopPerEndpoint is greater than 1, the different eventLoop might be used which is inefficient.

the different eventLoop might be used which is inefficient.

Although it is inefficient I think it would be good to try a different connection than the connection that has already failed.

trustin · 2023-11-08T02:31:52Z

And can we also add an integration test that involves RetryingClient to ensure the fix actually fixes the problem described in the PR description?

codecov · 2023-11-08T02:46:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ff9f2e0) 0.00% compared to head (d73eabc) 73.95%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5290       +/-   ##
===========================================
+ Coverage        0   73.95%   +73.95%     
- Complexity      0    20104    +20104     
===========================================
  Files           0     1730     +1730     
  Lines           0    74139    +74139     
  Branches        0     9460     +9460     
===========================================
+ Hits            0    54831    +54831     
- Misses          0    14832    +14832     
- Partials        0     4476     +4476

Files	Coverage Δ
...a/internal/client/DefaultClientRequestContext.java	`90.90% <100.00%> (ø)`

... and 1729 files with indirect coverage changes

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

jrhee17

Happy with the changes once the CI passes 👍 Thanks @ikhoon 🙇 👍 🙇

ikhoon · 2023-11-08T05:57:02Z

I've reverted all changes in *RequestContextBuilders since it caused additional side effects in tests which would be out of the scope of this PR.

Let me handle it in a separate PR to build a RequestContext with an EndpointGroup

jrhee17 · 2023-11-08T05:58:06Z

By the way, was this ever commented on/addressed?
#5290 (comment)

trustin

Just once nit - LGTM 🙇

core/src/test/java/com/linecorp/armeria/client/retry/RetyingClientEventLoopSchedulerTest.java

ikhoon · 2023-11-08T06:21:12Z

By the way, was this ever commented on/addressed?

Commented. JFYI, if condition has changed.

trustin

minwoox

Thanks for fixing this. 🙇

ikhoon added the defect label Nov 7, 2023

ikhoon added this to the 1.26.2 milestone Nov 7, 2023

ikhoon requested review from jrhee17, minwoox and trustin as code owners November 7, 2023 14:59

ikhoon changed the title ~~Fix a bug where a connection may be not reused when using a retrying client.~~ Fix a bug where a connection may be not reused when using RetryingClient Nov 7, 2023

minwoox reviewed Nov 8, 2023

View reviewed changes

jrhee17 reviewed Nov 8, 2023

View reviewed changes

ikhoon added 2 commits November 8, 2023 11:11

Assign log first

e7b45b0

lint

03a22db

trustin reviewed Nov 8, 2023

View reviewed changes

ikhoon added 4 commits November 8, 2023 12:47

Add integration tests

9225b62

Fix a bug in ClientRequestContextBuilder

8dacde6

Make return the specified eventloop in AbstractRequestContextBuilder

3582897

Revert changes in RequestContextBuilders

d0dff0c

jrhee17 approved these changes Nov 8, 2023

View reviewed changes

clean up test code

39b504e

trustin approved these changes Nov 8, 2023

View reviewed changes

core/src/test/java/com/linecorp/armeria/client/retry/RetyingClientEventLoopSchedulerTest.java Outdated Show resolved Hide resolved

ikhoon added 2 commits November 8, 2023 15:22

Use isSameAs

da3b6ca

Missing commit

422583d

trustin approved these changes Nov 8, 2023

View reviewed changes

use ctx.eventLoop()

d73eabc

minwoox approved these changes Nov 8, 2023

View reviewed changes

minwoox merged commit 88ec879 into line:main Nov 8, 2023
15 checks passed

ikhoon deleted the retry-client-eventloop branch June 28, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug where a connection may be not reused when using `RetryingClient` #5290

Fix a bug where a connection may be not reused when using `RetryingClient` #5290

ikhoon commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

minwoox Nov 8, 2023

jrhee17 left a comment

jrhee17 Nov 8, 2023

ikhoon Nov 8, 2023

jrhee17 Nov 8, 2023

ikhoon Nov 8, 2023

trustin Nov 8, 2023

ikhoon Nov 8, 2023

minwoox Nov 8, 2023

ikhoon Nov 8, 2023

trustin commented Nov 8, 2023

codecov bot commented Nov 8, 2023 •

edited

Loading

jrhee17 left a comment

ikhoon commented Nov 8, 2023

jrhee17 commented Nov 8, 2023

trustin left a comment

ikhoon commented Nov 8, 2023

trustin left a comment

minwoox left a comment

Fix a bug where a connection may be not reused when using RetryingClient #5290

Fix a bug where a connection may be not reused when using RetryingClient #5290

Conversation

ikhoon commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 • edited Loading

🔍 Build Scan® (commit: d73eabc)

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Nov 8, 2023

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon commented Nov 8, 2023

jrhee17 commented Nov 8, 2023

trustin left a comment

Choose a reason for hiding this comment

ikhoon commented Nov 8, 2023

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Fix a bug where a connection may be not reused when using `RetryingClient` #5290

Fix a bug where a connection may be not reused when using `RetryingClient` #5290

github-actions bot commented Nov 7, 2023 •

edited

Loading

🔍 Build Scan® (commit: `d73eabc`)

codecov bot commented Nov 8, 2023 •

edited

Loading