Set RequestContext.isTimedOut(true) on DNS, session, write timeout #5156

injae-kim · 2023-08-29T14:16:09Z

Related issue #4935

Motivation:

We need to make sure CancellationScheduler.finishNow(TimeoutException) is invoked on more timeout types,
such as session/connection/DNS timeout so that true is set to ctx.isTimedOut().

On A DnsTimeoutException should also set true to ctx.isTimedOut(). #4935, we found that RequestContext.isTimedout is not set true on DNS, session creation, write timeout so we need to fix it

Modifications:

Set RequestContext.isTimedOut(true) on DNS, session, write timeout by ctx.cancel(TimeoutException.class)

Result:

Now RequestContext.isTimedout is set true well on DNS, session, write timeout too
Close A DnsTimeoutException should also set true to ctx.isTimedOut(). #4935

github-actions · 2023-08-29T14:49:28Z

🔍 Build Scan® (commit: `24cffdf`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/clxvkstw6p5dy
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/r22f6hwjvocdk
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/fth7o5hmyftbi
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/grez5if7iuxmo
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/2uibvm3dg6zky
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/625ltuwy2gv5y
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/wepcqoajnf3tm
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/m55la3jeimwok

codecov · 2023-08-29T15:19:07Z

Codecov Report

Attention: Patch coverage is 50.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 73.92%. Comparing base (863e27c) to head (084bf58).
Report is 387 commits behind head on main.

Files	Patch %	Lines
...rmeria/client/SessionCreationTimeoutException.java	0.00%	10 Missing ⚠️
...ia/client/SessionProtocolNegotiationException.java	66.66%	1 Missing and 1 partial ⚠️
.../linecorp/armeria/client/ClientRequestContext.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5156      +/-   ##
============================================
- Coverage     74.25%   73.92%   -0.34%     
- Complexity    19825    20023     +198     
============================================
  Files          1699     1722      +23     
  Lines         73046    73899     +853     
  Branches       9357     9406      +49     
============================================
+ Hits          54239    54628     +389     
- Misses        14371    14820     +449     
- Partials       4436     4451      +15

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

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

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

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

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

injae-kim · 2023-09-02T05:15:04Z

core/src/test/java/com/linecorp/armeria/client/HttpClientFactoryTest.java

+                        .build();
+
+                assertThat(reqCtx.isTimedOut()).isFalse();
+                assertThatThrownBy(() -> client.unwrap().execute(reqCtx, req).aggregate().join())


Suggested change

assertThatThrownBy(() -> client.unwrap().execute(reqCtx, req).aggregate().join())

assertThatThrownBy(() -> client.get("/").aggregate().join())

Hmm when I use WebClient.get("/") directly, reqCtx.whenResponseCancelled().join() not done infinitely 🤔

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

Line 144 in 863e27c

responseCancellationScheduler.init(eventLoop(), noopCancellationTask, 0, /* server */ false);

I guess ClientRequestContextBuilder().build() init cancellationScheduler immediately, but WebClient didn't.

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

Lines 228 to 229 in 863e27c

this.responseCancellationScheduler =

new CancellationScheduler(TimeUnit.MILLISECONDS.toNanos(responseTimeoutMillis));

armeria/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java

Line 311 in 863e27c

addPendingTask(() -> finishNow0(cause));

On WebClient, it uses DefaultClientRequestContext and it doesn't init() the ClientRequestContext's cancellationScheduler immediately.
Just add task (e.g. ctx.cancel(cause)) to pending task and execute it on init() lazily later like below.

armeria/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java

Lines 129 to 132 in 863e27c

if (pendingTaskUpdater.compareAndSet(this, pendingTask, noopPendingTask)) {

if (pendingTask != null) {

pendingTask.run();

}

But to get reqCtx.whenCancelled() completion, we need to init() cancellationScheduler.

So I used client.unwarp().execuite(reqCtx) to use ClientRequestContext.builder().build() to init cancellationScheduler~!

I'm not sure it's correct, so feel free to share your opinion! I'll update PR 🙇

I believe you can just use Clients.newContextCaptor() if you would like to get ahold of the RequestContext being used

oh I tried to use Clients.newContextCaptor() but cancellationScheduler is still not init() by this way so ClientRequestContext.isTimedOut() return false 😅

jrhee17

Looks good overall! Left some minor comments 🙇

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

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

jrhee17 · 2023-09-04T09:07:42Z

core/src/test/java/com/linecorp/armeria/client/HttpClientFactoryTest.java

+                        .build();
+
+                assertThat(reqCtx.isTimedOut()).isFalse();
+                assertThatThrownBy(() -> client.unwrap().execute(reqCtx, req).aggregate().join())


I believe you can just use Clients.newContextCaptor() if you would like to get ahold of the RequestContext being used

jrhee17

Took another look at this PR.

The way I see it there are two options (or actually three):

Use log.responseCause to determine whether a request/response has timed out (or actually log().getIfAvailable(RequestLogProperty.RESPONSE_CAUSE).responseCause();)
Modify responseCancellationScheduler so that either
1.init is called at an earlier timing. Ideally, this would involve scheduling timeouts for each step (dns, connect, write, etc..) using ctx.cancellationScheduler.
2. modify the behavior so that cancel() also executes pending tasks.

I think 1 or 2-2 is probably a realistic direction for this PR. Let me discuss with the other tomorrow morning 🙇

injae-kim · 2023-10-10T06:23:30Z

I think 1 or 2-2 is probably a realistic direction for this PR. Let me discuss with the other tomorrow morning

If there's some discussion progress about direction, feel free to share to me~! 🙇

minwoox · 2023-10-10T07:06:00Z

We have decided to go with the second option. @jrhee17 is laying the groundwork for that.
#5212
He will let you know when it's done. 😉

injae-kim · 2023-10-10T08:52:33Z

We have decided to go with the second option

aha~ I checked #5212. thanks for sharing!
feel free to let me know when it's ready to go with the 2. option~! 🙇

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

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

minwoox · 2024-04-02T01:08:05Z

Since #5212 has been completed, perhaps we can resume this work now?
cc @jrhee17

injae-kim · 2024-04-04T09:06:12Z

oh~ I'll update this PR on this weekend. thanks! :)

injae-kim · 2024-07-05T08:00:29Z

Since #5212 has been completed, perhaps we can resume this work now?

@jrhee17, I updated this PR to latest based on #5212~! PTAL 🙇

jrhee17

Changes look good to me 👍 👍 👍

minwoox

Just one nit. Thank! 😄

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

ikhoon

Thanks, @injae-kim! 🙇‍♂️🙇‍♂️

injae-kim requested review from ikhoon, jrhee17, minwoox and trustin as code owners August 29, 2023 14:16

injae-kim force-pushed the set-reqctx-istimedout-true branch from ae89c0b to 9396937 Compare August 29, 2023 14:42

injae-kim commented Aug 30, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/SessionCreationTimeoutException.java Outdated Show resolved Hide resolved

injae-kim force-pushed the set-reqctx-istimedout-true branch 2 times, most recently from 92426fb to d58b565 Compare September 1, 2023 08:04

injae-kim commented Sep 2, 2023

View reviewed changes

jrhee17 reviewed Sep 4, 2023

View reviewed changes

jrhee17 reviewed Sep 25, 2023

View reviewed changes

minwoox added this to the 1.27.0 milestone Oct 11, 2023

ikhoon reviewed Oct 17, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/client/SessionCreationTimeoutException.java Outdated Show resolved Hide resolved

ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024

jrhee17 modified the milestones: 1.28.0, 1.29.0 Apr 2, 2024

github-actions bot added the Stale label May 5, 2024

minwoox modified the milestones: 1.29.0, 1.30.0 May 21, 2024

github-actions bot removed the Stale label May 23, 2024

github-actions bot added the Stale label Jun 22, 2024

injae-kim force-pushed the set-reqctx-istimedout-true branch from 084bf58 to a8aa4a6 Compare July 4, 2024 15:46

Set RequestContext.isTimedOut(true) on DNS, session, write timeout

6c76245

injae-kim force-pushed the set-reqctx-istimedout-true branch from a8aa4a6 to 6c76245 Compare July 4, 2024 15:54

github-actions bot removed the Stale label Jul 5, 2024

injae-kim requested a review from jrhee17 July 5, 2024 07:59

jrhee17 approved these changes Jul 10, 2024

View reviewed changes

minwoox approved these changes Jul 19, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/SessionProtocolNegotiationException.java Outdated Show resolved Hide resolved

Remove requireNonNull on @nullable field

24cffdf

ikhoon approved these changes Aug 1, 2024

View reviewed changes

ikhoon merged commit 4c6f0ff into line:main Aug 2, 2024
15 checks passed

ikhoon mentioned this pull request Aug 2, 2024

ClientRequestContext#cancel cancels the associated request immediately #5800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set RequestContext.isTimedOut(true) on DNS, session, write timeout #5156

Set RequestContext.isTimedOut(true) on DNS, session, write timeout #5156

injae-kim commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

injae-kim Sep 2, 2023

jrhee17 Sep 4, 2023

injae-kim Sep 11, 2023

jrhee17 left a comment

jrhee17 Sep 4, 2023

jrhee17 left a comment •

edited

Loading

injae-kim commented Oct 10, 2023

minwoox commented Oct 10, 2023

injae-kim commented Oct 10, 2023

minwoox commented Apr 2, 2024

injae-kim commented Apr 4, 2024

injae-kim commented Jul 5, 2024

jrhee17 left a comment

minwoox left a comment

ikhoon left a comment

	assertThatThrownBy(() -> client.unwrap().execute(reqCtx, req).aggregate().join())
	assertThatThrownBy(() -> client.get("/").aggregate().join())

	this.responseCancellationScheduler =
	new CancellationScheduler(TimeUnit.MILLISECONDS.toNanos(responseTimeoutMillis));

	if (pendingTaskUpdater.compareAndSet(this, pendingTask, noopPendingTask)) {
	if (pendingTask != null) {
	pendingTask.run();
	}

Set RequestContext.isTimedOut(true) on DNS, session, write timeout #5156

Set RequestContext.isTimedOut(true) on DNS, session, write timeout #5156

Conversation

injae-kim commented Aug 29, 2023

Motivation:

Modifications:

Result:

github-actions bot commented Aug 29, 2023 • edited Loading

🔍 Build Scan® (commit: 24cffdf)

codecov bot commented Aug 29, 2023 • edited Loading

Codecov Report

injae-kim Sep 2, 2023

Choose a reason for hiding this comment

jrhee17 Sep 4, 2023

Choose a reason for hiding this comment

injae-kim Sep 11, 2023

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Sep 4, 2023

Choose a reason for hiding this comment

jrhee17 left a comment • edited Loading

Choose a reason for hiding this comment

injae-kim commented Oct 10, 2023

minwoox commented Oct 10, 2023

injae-kim commented Oct 10, 2023

minwoox commented Apr 2, 2024

injae-kim commented Apr 4, 2024

injae-kim commented Jul 5, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2023 •

edited

Loading

🔍 Build Scan® (commit: `24cffdf`)

codecov bot commented Aug 29, 2023 •

edited

Loading

jrhee17 left a comment •

edited

Loading