Introduce the `CancellationScheduler` interface #5220

jrhee17 · 2023-10-04T10:23:43Z

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.

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

Line 550 in 8e139d9

null, REQUEST_OPTIONS_FOR_UPGRADE_REQUEST, noopResponseCancellationScheduler,

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 CancellationScheduler finishes even if not started #5212.
Cleaner code overall.

codecov · 2023-10-04T10:57:47Z

Codecov Report

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

Comparison is base (9568d56) 0.00% compared to head (0bc6edd) 74.05%.
Report is 12 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5220       +/-   ##
===========================================
+ Coverage        0   74.05%   +74.05%     
- Complexity      0    19968    +19968     
===========================================
  Files           0     1717     +1717     
  Lines           0    73576    +73576     
  Branches        0     9364     +9364     
===========================================
+ Hits            0    54489    +54489     
- Misses          0    14647    +14647     
- Partials        0     4440     +4440

Files	Coverage Δ
...rp/armeria/client/ClientRequestContextBuilder.java	`80.76% <100.00%> (ø)`
...armeria/client/HttpClientPipelineConfigurator.java	`78.96% <100.00%> (ø)`
...a/internal/client/DefaultClientRequestContext.java	`90.72% <100.00%> (ø)`
...armeria/internal/common/CancellationScheduler.java	`100.00% <100.00%> (ø)`
.../internal/server/DefaultServiceRequestContext.java	`86.33% <100.00%> (ø)`
...p/armeria/server/ServiceRequestContextBuilder.java	`89.47% <100.00%> (ø)`
...ria/internal/common/NoopCancellationScheduler.java	`45.00% <45.00%> (ø)`
.../internal/common/DefaultCancellationScheduler.java	`77.38% <77.38%> (ø)`

... and 1709 files with indirect coverage changes

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

github-actions · 2023-10-04T11:37:14Z

🔍 Build Scan® (commit: `0bc6edd`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/hfffcwjsflmvm
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/35cbev2tkvd3i
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/csuasieht76tc
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/n5xytddyjrzt6
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/2xnmynn3kvtgg
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/sxckry2z5xjr4
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/cblnpfzrturkk
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/mvgh24y4qero6

minwoox

Looks great! 👍

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

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

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

minwoox · 2023-10-10T06:39:05Z

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

+import io.netty.util.concurrent.EventExecutor;
+
+@SuppressWarnings("UnstableApiUsage")
+final class DefaultCancellationScheduler implements CancellationScheduler {


I didn't look at this class because I think there's no logic change. Please let me know If it's. 😎

Did a diff with the main branch so that review can be done more easily

user@AL02437565 Projects % diff upstream-armeria/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java armeria/core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java 39,40c39 < @SuppressWarnings("UnstableApiUsage") < public final class CancellationScheduler { --- > final class DefaultCancellationScheduler implements CancellationScheduler { 42c41 < private static final AtomicReferenceFieldUpdater<CancellationScheduler, CancellationFuture> --- > private static final AtomicReferenceFieldUpdater<DefaultCancellationScheduler, CancellationFuture> 44c43 < CancellationScheduler.class, CancellationFuture.class, "whenCancelling"); --- > DefaultCancellationScheduler.class, CancellationFuture.class, "whenCancelling"); 46c45 < private static final AtomicReferenceFieldUpdater<CancellationScheduler, CancellationFuture> --- > private static final AtomicReferenceFieldUpdater<DefaultCancellationScheduler, CancellationFuture> 48c47 < CancellationScheduler.class, CancellationFuture.class, "whenCancelled"); --- > DefaultCancellationScheduler.class, CancellationFuture.class, "whenCancelled"); 50c49 < private static final AtomicReferenceFieldUpdater<CancellationScheduler, TimeoutFuture> --- > private static final AtomicReferenceFieldUpdater<DefaultCancellationScheduler, TimeoutFuture> 52c51 < CancellationScheduler.class, TimeoutFuture.class, "whenTimingOut"); --- > DefaultCancellationScheduler.class, TimeoutFuture.class, "whenTimingOut"); 54c53 < private static final AtomicReferenceFieldUpdater<CancellationScheduler, TimeoutFuture> --- > private static final AtomicReferenceFieldUpdater<DefaultCancellationScheduler, TimeoutFuture> 56c55 < CancellationScheduler.class, TimeoutFuture.class, "whenTimedOut"); --- > DefaultCancellationScheduler.class, TimeoutFuture.class, "whenTimedOut"); 58c57 < private static final AtomicReferenceFieldUpdater<CancellationScheduler, Runnable> --- > private static final AtomicReferenceFieldUpdater<DefaultCancellationScheduler, Runnable> 60c59 < CancellationScheduler.class, Runnable.class, "pendingTask"); --- > DefaultCancellationScheduler.class, Runnable.class, "pendingTask"); 62,63c61,62 < private static final AtomicLongFieldUpdater<CancellationScheduler> pendingTimeoutNanosUpdater = < AtomicLongFieldUpdater.newUpdater(CancellationScheduler.class, "pendingTimeoutNanos"); --- > private static final AtomicLongFieldUpdater<DefaultCancellationScheduler> pendingTimeoutNanosUpdater = > AtomicLongFieldUpdater.newUpdater(DefaultCancellationScheduler.class, "pendingTimeoutNanos"); 93c92 < public CancellationScheduler(long timeoutNanos) { --- > DefaultCancellationScheduler(long timeoutNanos) { 99c98 < * Initializes this {@link CancellationScheduler}. --- > * Initializes this {@link DefaultCancellationScheduler}. 100a100 > @Override 137a138 > @Override 141a143 > @Override 176a179 > @Override 295a299 > @Override 299a304 > @Override 328a334 > @Override 336a343 > @Override 341a349 > @Override 345a354 > @Override 349a359 > @Override 362a373 > @Override 375a387 > @Override 394a407 > @Override 413a427 > @Override 495,517d508 < enum State { < INIT, < INACTIVE, < SCHEDULED, < FINISHING, < FINISHED < } < < /** < * A cancellation task invoked by the scheduler when its timeout exceeds or invoke by the user. < */ < public interface CancellationTask { < /** < * Returns {@code true} if the cancellation task can be scheduled. < */ < boolean canSchedule(); < < /** < * Invoked by the scheduler with the cause of cancellation. < */ < void run(Throwable cause); < } < user@AL02437565 Projects %

So I guess there's no logic changes then. 😉

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

minwoox

Looks great, thanks! 😄

minwoox · 2023-10-11T09:58:53Z

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

+
+    @Override
+    public long timeoutNanos() {
+        return 0;


~~Will MAX_VALUE be more appropriate?~~
Never mind, 0 will be fine.

ikhoon · 2023-10-11T11:56:19Z

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

-            }
-        }
+    static CancellationScheduler finished(boolean server) {
+        final CancellationScheduler cancellationScheduler = of(0);


minor) Should we cache and reuse?

if (server) { return serverfinishedCancellationScheduler; } else { ... }

ikhoon

Thanks, @jrhee17 👍👍

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.

refactor cancellation scheduler

8f2ee36

jrhee17 added the improvement label Oct 4, 2023

jrhee17 added this to the 1.26.0 milestone Oct 4, 2023

jrhee17 changed the title ~~Refactor cancellation scheduler~~ Introduce the CancellationScheduler interface Oct 4, 2023

jrhee17 marked this pull request as ready for review October 4, 2023 14:02

jrhee17 requested review from ikhoon, minwoox and trustin as code owners October 4, 2023 14:02

minwoox reviewed Oct 10, 2023

View reviewed changes

address comments by @minwoox

7573540

minwoox approved these changes Oct 11, 2023

View reviewed changes

ikhoon reviewed Oct 11, 2023

View reviewed changes

address comment by @ikhoon

0bc6edd

jrhee17 force-pushed the refactor/init-cancel-scheduler-1 branch from 6864add to 0bc6edd Compare October 17, 2023 10:07

ikhoon approved these changes Oct 17, 2023

View reviewed changes

jrhee17 merged commit 6a7f457 into line:main Oct 18, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the `CancellationScheduler` interface #5220

Introduce the `CancellationScheduler` interface #5220

jrhee17 commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

minwoox left a comment

minwoox Oct 10, 2023

jrhee17 Oct 11, 2023

minwoox Oct 11, 2023 •

edited

Loading

minwoox left a comment

minwoox Oct 11, 2023 •

edited

Loading

ikhoon Oct 11, 2023

ikhoon left a comment

Introduce the CancellationScheduler interface #5220

Introduce the CancellationScheduler interface #5220

Conversation

jrhee17 commented Oct 4, 2023 • edited Loading

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

github-actions bot commented Oct 4, 2023 • edited Loading

🔍 Build Scan® (commit: 0bc6edd)

minwoox left a comment

Choose a reason for hiding this comment

minwoox Oct 10, 2023

Choose a reason for hiding this comment

jrhee17 Oct 11, 2023

Choose a reason for hiding this comment

minwoox Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

ikhoon Oct 11, 2023

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Introduce the `CancellationScheduler` interface #5220

Introduce the `CancellationScheduler` interface #5220

jrhee17 commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

🔍 Build Scan® (commit: `0bc6edd`)

minwoox Oct 11, 2023 •

edited

Loading

minwoox Oct 11, 2023 •

edited

Loading