Let GrpcService specify a maximum bound for grpc-timeout

[jrhee17]

Currently, armeria has a useClientTimeoutHeader option to decide whether to respect the grpc-timeout request header or not.

Since useClientTimeoutHeader = true delegates the timeout value to the client-side, it is potentially dangerous.
On the other hand, setting useClientTimeoutHeader = false completely ignores the client header and Armeria's requestTimeout is used.

We may want to offer users a middle ground: Armeria server tries to respect the grpc-timeout header, but clamps the timeout according to a bound.

In order to support this, we may deprecate the previous useClientTimeoutHeader and accept a function which determines how to decide the request timeout. This might also be useful if users want to set timeouts depending on the remote.

@Deprecated
GrpcServiceBuilder useClientTimeoutHeader(boolean useClientTimeoutHeader) {}

interface GrpcRequestTimeoutMapper {

  long map(RequestHeaders headers, ServiceRequestContext ctx);

  static GrpcRequestTimeoutMapper USE_CLIENT_TIMEOUT_HEADER = (headers, ctx) -> {
    // https://github.com/line/armeria/blob/e263d76deff8f936a58db69de6ad63d0c0458407/grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java#L226-L254    
  }

  static GrpcRequestTimeoutMapper USE_SERVER_REQUEST_TIMEOUT = (ignored, ctx) -> TimeUnit.MILLISECONDS.toNanos(ctx.requestTimeoutMillis());
}

GrpcServiceBuilder requestTimeout(GrpcRequestTimeoutMapper mapper) {}

...
GrpcRequestTimeoutMapper mapper;
...
HttpResponse doPost {
  // returns nanos
  long timeout = mapper.map(headers, ctx);
  if (timeout <= 0) {
    ctx.clearRequestTimeout();
  } else {
    ctx.setRequestTimeout(TimeoutMode.SET_FROM_NOW, Duration.ofNanos(timeout));
  }
}
...

ref: https://discord.com/channels/1087271586832318494/1245876539682324642/1245948161835667528

[ikhoon]

grpc-timeout is not just a number. A suffix representing a time unit such as s, m or n is appended to the value. It would be a hassle to parse the value.

How about passing the parsed grpc-timeout value to the interface?

interface GrpcClientTimeoutHandler {
    static GrpcClientTimeoutHandler enabled() {
       return Function.identity();
    }
    
    static GrpcClientTimeoutHandler disabled() {
       return (ctx, timeout) -> -1;
    }

    static GrpcClientTimeoutHandler withBuffer(long bufferMillis) {
       return (ctx, timeout) -> timeout + buffer;
    }

    long apply(ServiceRequestContext ctx, long timeoutMillis);
}

[jrhee17]

How about passing the parsed grpc-timeout value to the interface?

The original intention was that users may want to know other header values when clamping the timeout.
However, I realized that this information is in ctx.request().headers() anyways, so the interface you suggested seems better. (The only concern I have is nanos vs. millis)

[jrhee17]

Also possibly related #3155

[ikhoon]

er. (The only concern I have is nanos vs. millis

If it is considerable, Duration may be an alternative.

[Dogacel]

I wonder if any of the server side flags affect the timeout behavior?

• defaultRequestTimeoutMillis
• defaultResponseTimeoutMillis

etc. IIRC some of those caused connection to be terminated pre-maturely before client initiates a timeout.

[ikhoon]

Timeout does not cause connections to be terminated. In HTTP/2, the timeout sends an error response and the connection will be reused.

I wonder if any of the server side flags affect the timeout behavior?

Possible. We are going to provide an option to allow grpc-timeout but the maximum value is limited by defaultRequestTimeoutMillis.