Skip to content

Commit

Permalink
Introduce the CancellationScheduler interface (line#5220)
Browse files Browse the repository at this point in the history
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.

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
  • Loading branch information
jrhee17 authored and Bue-von-hon committed Nov 10, 2023
1 parent 33eb987 commit acfbffe
Show file tree
Hide file tree
Showing 10 changed files with 705 additions and 521 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.linecorp.armeria.client;

import static com.linecorp.armeria.internal.common.CancellationScheduler.noopCancellationTask;
import static java.util.Objects.requireNonNull;

import java.net.InetSocketAddress;
Expand All @@ -35,12 +36,10 @@
import com.linecorp.armeria.common.util.SystemInfo;
import com.linecorp.armeria.internal.client.DefaultClientRequestContext;
import com.linecorp.armeria.internal.common.CancellationScheduler;
import com.linecorp.armeria.internal.common.CancellationScheduler.CancellationTask;

import io.micrometer.core.instrument.MeterRegistry;
import io.netty.buffer.ByteBufAllocator;
import io.netty.channel.EventLoop;
import io.netty.util.concurrent.ImmediateEventExecutor;

/**
* Builds a new {@link ClientRequestContext}. Note that it is not usually required to create a new context by
Expand All @@ -49,27 +48,6 @@
*/
public final class ClientRequestContextBuilder extends AbstractRequestContextBuilder {

private static final CancellationTask noopCancellationTask = new CancellationTask() {
@Override
public boolean canSchedule() {
return true;
}

@Override
public void run(Throwable cause) { /* no-op */ }
};

/**
* A cancellation scheduler that has been finished.
*/
static final CancellationScheduler noopResponseCancellationScheduler = new CancellationScheduler(0);

static {
noopResponseCancellationScheduler
.init(ImmediateEventExecutor.INSTANCE, noopCancellationTask, 0, /* server */ false);
noopResponseCancellationScheduler.finishNow();
}

@Nullable
private Endpoint endpoint;
private ClientOptions options = ClientOptions.of();
Expand Down Expand Up @@ -136,9 +114,9 @@ public ClientRequestContext build() {

final CancellationScheduler responseCancellationScheduler;
if (timedOut()) {
responseCancellationScheduler = noopResponseCancellationScheduler;
responseCancellationScheduler = CancellationScheduler.finished(false);
} else {
responseCancellationScheduler = new CancellationScheduler(0);
responseCancellationScheduler = CancellationScheduler.of(0);
final CountDownLatch latch = new CountDownLatch(1);
eventLoop().execute(() -> {
responseCancellationScheduler.init(eventLoop(), noopCancellationTask, 0, /* server */ false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.linecorp.armeria.client;

import static com.google.common.base.MoreObjects.firstNonNull;
import static com.linecorp.armeria.client.ClientRequestContextBuilder.noopResponseCancellationScheduler;
import static com.linecorp.armeria.common.SessionProtocol.H1;
import static com.linecorp.armeria.common.SessionProtocol.H1C;
import static com.linecorp.armeria.common.SessionProtocol.H2;
Expand Down Expand Up @@ -57,6 +56,7 @@
import com.linecorp.armeria.internal.client.UserAgentUtil;
import com.linecorp.armeria.internal.common.ArmeriaHttp2HeadersDecoder;
import com.linecorp.armeria.internal.common.ArmeriaHttpUtil;
import com.linecorp.armeria.internal.common.CancellationScheduler;
import com.linecorp.armeria.internal.common.ReadSuppressingHandler;
import com.linecorp.armeria.internal.common.TrafficLoggingHandler;
import com.linecorp.armeria.internal.common.util.ChannelUtil;
Expand Down Expand Up @@ -547,7 +547,7 @@ public void onComplete() {}
com.linecorp.armeria.common.HttpMethod.OPTIONS,
RequestTarget.forClient("*"), ClientOptions.of(),
HttpRequest.of(com.linecorp.armeria.common.HttpMethod.OPTIONS, "*"),
null, REQUEST_OPTIONS_FOR_UPGRADE_REQUEST, noopResponseCancellationScheduler,
null, REQUEST_OPTIONS_FOR_UPGRADE_REQUEST, CancellationScheduler.noop(),
System.nanoTime(), SystemInfo.currentTimeMicros());

// NB: No need to set the response timeout because we have session creation timeout.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ private DefaultClientRequestContext(
responseTimeoutMillis = options().responseTimeoutMillis();
}
this.responseCancellationScheduler =
new CancellationScheduler(TimeUnit.MILLISECONDS.toNanos(responseTimeoutMillis));
CancellationScheduler.of(TimeUnit.MILLISECONDS.toNanos(responseTimeoutMillis));
} else {
this.responseCancellationScheduler = responseCancellationScheduler;
}
Expand Down Expand Up @@ -504,7 +504,7 @@ private DefaultClientRequestContext(DefaultClientRequestContext ctx,
log = RequestLog.builder(this);
log.startRequest();
responseCancellationScheduler =
new CancellationScheduler(TimeUnit.MILLISECONDS.toNanos(ctx.responseTimeoutMillis()));
CancellationScheduler.of(TimeUnit.MILLISECONDS.toNanos(ctx.responseTimeoutMillis()));
writeTimeoutMillis = ctx.writeTimeoutMillis();
maxResponseLength = ctx.maxResponseLength();

Expand Down
Loading

0 comments on commit acfbffe

Please sign in to comment.