`CancellationScheduler` finishes even if not started #5212

jrhee17 · 2023-09-26T06:52:06Z

Motivation:

Currently, a CancellationScheduler doesn't complete the callback ClientRequestContext#whenResponseCancelled if CancellationScheduler#init is not called.

This isn't a problem for server side since CancellationScheduler#init is called immediately. However, for client side CancellationScheduler#init is called when a HttpResponse is subscribed to. For this reason, even if we call ClientRequestContext#cancel the callback ClientRequestContext#whenResponseCancelled is never completed.

We have recently decided that the semantics of "ClientRequestContext#cancel" means:

Client side: A failure when the response has not been fully received
Server side: A failure then the response has not been fully sent

This means we should be able to finish a CancellationScheduler although it hasn't been initialized yet.
In order to achieve this, I propose that the following changes are made.

An event loop is assigned to CancellationScheduler at an earlier timing so that ctx.cancel is run from an event loop.
- This is done so CancellationScheduler#finish can be called before CancellationScheduler#start without worrying about race conditions
The previous CancellationScheduler#init method has been divided into two methods: CancellationScheduler#init and CancellationScheduler#start
- CancellationScheduler#init initializes the CancellationScheduler with an EventExecutor
- CancellationScheduler#start actually starts the cancellation timer if the timeout > 0
DefaultClientRequestContext#init is called as soon as an EventExecutor is assigned
With the new semantics of ctx#cancel, I believe the request should be cancellable from more locations in the future. I propose that:
- If a timeout has already been reached when CancellationScheduler#start is called, then the task is executed immediately.
- If a task already exists, the task is updated another invocation of CancellationScheduler#start

I imagine we can later introduce a responseTimeStartTiming parameter, which dictates where to call CancellationScheduler#start from.

enum ResponseTimeTiming {
  CTX_INIT, // calls CancellationScheduler#start right after `ctx.init` is done
  DELEGATE_EXECUTE, // calls CancellationScheduler#start right after `HttpClientDelegate#execute` is done.
  RESPONSE_HEADERS // calls CancellationScheduler#start when the first response header is received (current behavior)
}

Each location may also update the cancellation task since how to cancel from each step will be different.

e.g. Cancellation from HttpClientDelegate#execute

public HttpResponse execute(ClientRequestContext ctx, ...) {
...
    final DecodedHttpResponse res = new DecodedHttpResponse(eventLoop);
    final CancellationScheduler cancellationScheduler = ...
    cancellationScheduler.start(new CancellationTask() {
        @Override
        public void run(Throwable cause) {
            final UnprocessedRequestException wrappedCause = UnprocessedRequestException.of(cause);
            handleEarlyRequestException(ctx, req, wrappedCause);
            res.close(wrappedCause);
        }
    });
...

This will be handled in a follow-up PR if it makes sense

Modifications:

Renamed CancellationScheduler#init to CancellationScheduler#initAndStart
- CancellationScheduler#init initializes the CancellationScheduler with an EventExecutor
- CancellationScheduler#start updates the task and schedules a timeout if not done already
CancellationScheduler#init is now called immediately when an event loop is updated in DefaultClientRequestContext
Modified CancellationScheduler such that finishNow executes from an event loop
- finishNow completes the state if even if it hasn't been started yet
(Cleanup) Removed the timeoutNanos parameter from CancellationScheduler#start since the parameter wasn't adding any value
(Cleanup) Changed the visibility of isInitialized since it wasn't being used anywhere
(Cleanup) Renamed CancellationScheduler#of to CancellationScheduler#ofClient, CancellationScheduler#ofServer for clarity

Result:

ctx.whenCancelled, ctx.whenCancelling is now completed even if the CancellationScheduler has not been initialized

github-actions · 2023-09-26T07:51:48Z

🔍 Build Scan® (commit: `e971dd9`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/rv3eamcfbdnma
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/yer4qlsbgzvdu
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/mxcfexla74yvi
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/an43aiftd23r6
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/kblu2bfrhyivi
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/fpfu2qyf5q6ua
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/dabgsohna56eo
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/p4etop7euuyem

This reverts commit afcd9d2.

This reverts commit af7537d.

codecov · 2023-09-27T17:04:11Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (c4c6b3d) 73.67% compared to head (57e1144) 73.93%.
Report is 11 commits behind head on main.

❗ Current head 57e1144 differs from pull request most recent head e971dd9. Consider uploading reports for the commit e971dd9 to get more accurate results

Files	Patch %	Lines
.../internal/common/DefaultCancellationScheduler.java	76.74%	4 Missing and 6 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5212      +/-   ##
============================================
+ Coverage     73.67%   73.93%   +0.25%     
- Complexity    20105    20114       +9     
============================================
  Files          1741     1730      -11     
  Lines         74353    74199     -154     
  Branches       9481     9474       -7     
============================================
+ Hits          54783    54858      +75     
+ Misses        15033    14852     -181     
+ Partials       4537     4489      -48

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

Motivation: While working on #5212, I realized that `noopResponseCancellationScheduler` is an already completed scheduler. When we create a `DefaultClientRequestContext` for the upgrade request, we use this scheduler. https://github.com/line/armeria/blob/8e139d972f5137f9d1805ddda4a7aeae60fecd40/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java#L550 However, actually what we want is a scheduler which never completes instead of an already completed scheduler. For this reason, I propose that we separate a - `CancellationScheduler#noop`: signifies a scheduler which never completes - `CancellationScheduler#finished`: signifies a scheduler which already completed Note that the upgrade request is already bound by `connectTimeout`, so it is safe to use a `CancellationScheduler` which never completes. Modifications: - Rename `CancellationScheduler` to `DefaultCancellationScheduler`, and reduce visibility - Introduce an interface `CancellationScheduler` - Introduce factory methods `CancellationScheduler#of`, `CancellationScheduler#noop`, `CancellationScheduler#finished` Result: - It is easier to handle #5212. - Cleaner code overall.

Motivation: While working on line#5212, I realized that `noopResponseCancellationScheduler` is an already completed scheduler. When we create a `DefaultClientRequestContext` for the upgrade request, we use this scheduler. https://github.com/line/armeria/blob/8e139d972f5137f9d1805ddda4a7aeae60fecd40/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java#L550 However, actually what we want is a scheduler which never completes instead of an already completed scheduler. For this reason, I propose that we separate a - `CancellationScheduler#noop`: signifies a scheduler which never completes - `CancellationScheduler#finished`: signifies a scheduler which already completed Note that the upgrade request is already bound by `connectTimeout`, so it is safe to use a `CancellationScheduler` which never completes. Modifications: - Rename `CancellationScheduler` to `DefaultCancellationScheduler`, and reduce visibility - Introduce an interface `CancellationScheduler` - Introduce factory methods `CancellationScheduler#of`, `CancellationScheduler#noop`, `CancellationScheduler#finished` Result: - It is easier to handle line#5212. - Cleaner code overall.

…scheduler

ikhoon

The overall direction of this PR looks good.

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

ikhoon · 2024-01-19T12:19:43Z

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

        } else {
-            addPendingTask(() -> finishNow0(cause));
+            start(noopCancellationTask);
+            finishNow0(cause);


ctx.cancel() called in a decorator before invoking delegate.execute() couldn't prevent the request headers from being written.
Would we abort the request in the following points if ctx.isCanceled() is true?

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

Line 64 in 5689068

final Throwable throwable = ClientPendingThrowableUtil.pendingThrowable(ctx);

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

Line 218 in 7911f59

if (handleEarlyCancellation(ctx, req, res)) {

I'm not decided on how we want to set up the dependency between log, HttpResponse, and ctx yet but I think this is one possibility.

I think overall we want to

Hook callbacks so that there is a dependency between the above three states

Check whether a single state is completed or not

For now, I'm imagining that:

If ctx is cancelled, then the corresponding HttpResponse is cancelled

We check for res.isDone overall for early cancellation

But I will have to explore this further. For now, I think this PR doesn't change the previous behavior so it should probably be safe to merge.

Do you prefer this be done in this PR as well?

For now, I'm imagining that:

Sounds good.

The case where ctx.cancel() is called before a normal CancellationTask is registered can be considered a separate issue.

ikhoon

Thanks, @jrhee17! 🙇‍♂️🙇‍♂️

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

minwoox

Changes look straightforward, thanks! 👍
(I wanted to try writing this sentence. 😆 )

ikhoon · 2024-01-23T08:40:33Z

Okay to merge this PR if CI builds pass. 😉

jrhee17 · 2024-01-23T10:45:14Z

Thanks for the review all 🙇

jrhee17 added 2 commits September 26, 2023 15:32

minimal impl

af7537d

add tests

afcd9d2

jrhee17 added the improvement label Sep 26, 2023

jrhee17 added this to the 1.26.0 milestone Sep 26, 2023

jrhee17 added 9 commits September 27, 2023 15:53

Revert "add tests"

2d052cc

This reverts commit afcd9d2.

Revert "minimal impl"

48666e1

This reverts commit af7537d.

introduce a start method

0a75a07

asdf

56ffff8

divide init from start

653ba0c

allow immediate finish

d2700a1

extract an interface

af8252e

working version

bafb8a2

lint

96b5fc1

jrhee17 mentioned this pull request Oct 4, 2023

Introduce the CancellationScheduler interface #5220

Merged

minwoox mentioned this pull request Oct 10, 2023

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

Merged

jrhee17 modified the milestones: 1.26.0, 1.27.0 Oct 13, 2023

jrhee17 added 2 commits December 21, 2023 11:32

Merge remote-tracking branch 'origin/main' into refactor/init-cancel-…

2cdc080

…scheduler

update failing test

57e1144

jrhee17 force-pushed the refactor/init-cancel-scheduler branch from 4923d45 to 57e1144 Compare December 21, 2023 07:24

jrhee17 added 6 commits December 26, 2023 15:09

rename to initAndStart

108df5c

server is final

b60abce

tmp

4e50d7c

working version

742b40f

Merge remote-tracking branch 'origin/main' into refactor/init-cancel-…

f1ff541

…scheduler

minor fix

11baf70

jrhee17 added 2 commits January 18, 2024 10:49

remove timeout parameter in start method

e764192

minor cleanups

5689068

jrhee17 marked this pull request as ready for review January 18, 2024 07:06

jrhee17 requested review from ikhoon, minwoox and trustin as code owners January 18, 2024 07:06

jrhee17 changed the title ~~CancellationScheduler finishes even if not initialized~~ CancellationScheduler finishes even if not started Jan 18, 2024

ikhoon reviewed Jan 19, 2024

View reviewed changes

address comment by @ikhoon

6b3849e

ikhoon approved these changes Jan 23, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java Outdated Show resolved Hide resolved

address comment by @ikhoon

8750768

minwoox approved these changes Jan 23, 2024

View reviewed changes

update broken test

e971dd9

jrhee17 merged commit 154a7e4 into line:main Jan 23, 2024
14 checks passed

ikhoon added defect and removed improvement labels Jan 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`CancellationScheduler` finishes even if not started #5212

`CancellationScheduler` finishes even if not started #5212

jrhee17 commented Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading

ikhoon left a comment

ikhoon Jan 19, 2024 •

edited

Loading

jrhee17 Jan 23, 2024 •

edited

Loading

ikhoon Jan 23, 2024

ikhoon left a comment

minwoox left a comment

ikhoon commented Jan 23, 2024

jrhee17 commented Jan 23, 2024

CancellationScheduler finishes even if not started #5212

CancellationScheduler finishes even if not started #5212

Conversation

jrhee17 commented Sep 26, 2023 • edited Loading

github-actions bot commented Sep 26, 2023 • edited Loading

🔍 Build Scan® (commit: e971dd9)

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

jrhee17 Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

ikhoon Jan 23, 2024

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Jan 23, 2024

jrhee17 commented Jan 23, 2024

`CancellationScheduler` finishes even if not started #5212

`CancellationScheduler` finishes even if not started #5212

jrhee17 commented Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 26, 2023 •

edited

Loading

🔍 Build Scan® (commit: `e971dd9`)

codecov bot commented Sep 27, 2023 •

edited

Loading

ikhoon Jan 19, 2024 •

edited

Loading

jrhee17 Jan 23, 2024 •

edited

Loading