Add `ServiceOptions` and `@ServiceOption` #5574

seonWKim · 2024-04-07T12:41:12Z

Motivation:

Allow users to set default options for a specific service

Modifications:

Add ServiceOptions and @ServiceOption annotation to allow users specify default options

Result:

Users are able to set service specific options like below

final HttpService httpService = new HttpService() {
    ...

    @Override
    public ServiceOptions options() {
        return defaultServiceOptions;
    }
};

or use annotation

  final class MyService {

      @ServiceOption(requestTimeoutMillis = 11111, maxRequestLength = 1111,
              requestAutoAbortDelayMillis = 111)
      @Get("/test1")
      public HttpResponse test() {
          return HttpResponse.of("OK");
      }
   ...
}

Closes Add ServiceOptions to Service #5071. (If this resolves the issue.)

seonWKim · 2024-04-07T12:43:57Z

I think we need to decide what settings the ServiceOptions should support. For now, it only supports three settings which are:

requestTimeoutMillis
maxRequestLength
requestAutoAbortDelayMillis

seonWKim · 2024-04-07T12:46:07Z

Should we use the name ServiceOptions which is used to configure HttpService?
Because HttpService extends Service<HttpRequest, HttpResponse> it might be better to use HttpServiceOptions for HttpService rather than ServiceOptions. 🤔

github-actions · 2024-04-07T12:58:43Z

🔍 Build Scan® (commit: `a15ef81`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/lsxbfvbumys7s
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/qunllovw6zke6
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/qzudxustg4cja
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/3iwwhcbhzoujw
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/spg6ksr2yn7mo
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/72thoj34ool6y
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/m3t5hwmdusbey
build-macos-12-jdk-21	❌ (failure)	https://ge.armeria.dev/s/mcrxxzc7sy6hw

ikhoon · 2024-04-11T08:09:43Z

A service can be wrapped with a decorator. If the decorator does not override ServiceOptions, it should delegate options().

public abstract class SimpleDecoratingHttpService ... {
    @Override
    public ServiceOptions options() {
        return ((HttpService) unwrap()).options();
    }
}

ikhoon · 2024-04-11T08:12:29Z

it might be better to use HttpServiceOptions for HttpService rather than ServiceOptions

It may be better. Let use HttpServiceOptions for now. Other folks may have different opinions though.

core/src/main/java/com/linecorp/armeria/server/ServiceOptionsBuilder.java

core/src/main/java/com/linecorp/armeria/server/ServiceOptions.java

core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java

seonWKim · 2024-04-13T09:56:53Z

It may be better. Let use HttpServiceOptions for now. Other folks may have different opinions though.

I've renamed the class into HttpServiceOptions.

codecov · 2024-04-13T13:03:07Z

Codecov Report

Attention: Patch coverage is 79.01235% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 74.11%. Comparing base (b8eb810) to head (a15ef81).
Report is 208 commits behind head on main.

Files	Patch %	Lines
...om/linecorp/armeria/server/HttpServiceOptions.java	47.82%	12 Missing ⚠️
...corp/armeria/server/HttpServiceOptionsBuilder.java	78.57%	0 Missing and 3 partials ⚠️
...nal/server/annotation/DefaultAnnotatedService.java	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5574      +/-   ##
============================================
+ Coverage     73.95%   74.11%   +0.15%     
- Complexity    20115    21328    +1213     
============================================
  Files          1730     1855     +125     
  Lines         74161    78816    +4655     
  Branches       9465    10069     +604     
============================================
+ Hits          54847    58413    +3566     
- Misses        14837    15691     +854     
- Partials       4477     4712     +235

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

seonWKim · 2024-04-14T05:17:55Z

There were some conflicts with my origin and upstream, so I'd reset the commits and did a force push 😓

- OPTIONS -> DEFAULT OPTIONS - WEBSOCKET_DEFAULT -> WEB_SOCKET_DEFAULT_OPTIONS

core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java

- requestTimeoutMillis - maxRequestLength - requestAutoAbortDelayMillis

ikhoon · 2024-05-02T13:33:36Z

core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java

+ private static final HttpServiceOptions WEB_SOCKET_DEFAULT_OPTIONS = HttpServiceOptions
+ .builder()
+ .requestTimeoutMillis(WebSocketUtil.DEFAULT_REQUEST_RESPONSE_TIMEOUT_MILLIS)
+ .maxRequestLength(WebSocketUtil.DEFAULT_MAX_REQUEST_RESPONSE_LENGTH)
+ .requestAutoAbortDelayMillis(WebSocketUtil.DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS)
+ .build();


Should we move this to WebSocketService? I don't see we need to expose websocket() here.

interface WebSocketService { @Override default HttpServiceOptions options() { return DefaultWebSocketService.DEFAULT_OPTIONS; } }

Shouldn't we override the options() only in the DefaultWebSocketService? Below test seems to allow DEFAULT_OPTIONS for DefaultWebSocketService only.

@Test void shouldNotSetDefaultSettings() { final ServiceConfig serviceConfig = server.server().serviceConfigs().get(0); assertThat(serviceConfig.service().as(DelegatingWebSocketService.class)).isNotNull(); // The default settings for `WebSocketService` should be applied only to `DefaultWebSocketService`. assertThat(serviceConfig.requestTimeoutMillis()).isEqualTo(Flags.defaultRequestTimeoutMillis()); }

core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java

core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java

ikhoon · 2024-05-03T03:55:07Z

core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceConfigTest.java

@@ -40,6 +40,7 @@ void webSocketServiceDefaultConfigValues() {
 assertThat(serviceConfig.requestAutoAbortDelayMillis()).isEqualTo(
 WebSocketUtil.DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS);

+ // TODO: Should we allow default HttpServiceOptions to override the default values from virtualHostTemplate


The priority of HttpServiceOptions is between the values in ServiceBindingBuilder and VirtualHostBuilder.

The new priority of configurations will be: (Listed from high priority to low priority)

ServiceBindingBuilder (highest)

HttpServiceOptions (if exists)

VirtualHostBuilder

ServerBuilder (lowest)

Based on the new priority, sb.requestAutoAbortDelayMillis(1000) should not override HttpServiceOptions.requestAutoAbortDelayMillis() if requestAutoAbortDelayMillis > -1.

Should we test two scenarios?

Make sure that sb.requestAutoAbortDelayMillis(1000) does NOT override serviceConfig.requestAutoAbortDelayMillis() of WebSocketService

Make sure that sb.route().requestAutoAbortDelayMillis(1000) overrides WebSocketService.options().requestAutoAbortDelayMillis().

I've added above mentioned test scenarios in WebSocketServiceConfigTest#webSocketServiceOptionsPriority.

- Move DEFAULT_OPTIONS of webSocket under DefaultWebSocketService - Add test to verify priority of the options

…vice

minwoox · 2024-05-16T11:31:45Z

core/src/main/java/com/linecorp/armeria/server/HttpService.java

+ /**
+ * Returns the {@link HttpServiceOptions} of this {@link HttpService}.
+ */
+ @Nullable


What are your thoughts on making this non-nullable like other methods?

Sure, I'll make it as non-nullable as we can identify whether the user has set the value by checking whether the fields are set with values of -1 👍

core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java

minwoox · 2024-05-16T12:03:40Z

core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java

+ * Returns the server-side timeout of a request in milliseconds.
+ */
+ public HttpServiceOptionsBuilder requestTimeoutMillis(long requestTimeoutMillis) {
+ this.requestTimeoutMillis = requestTimeoutMillis;


We need to check the value:

checkArgument(requestTimeoutMillis >= 0, "requestTimeoutMillis: %s (expected: >= 0)", requestTimeoutMillis);

After that, we also need to modify these lines:
https://github.com/line/armeria/pull/5574/files#diff-2246967d4d345f650f2b3ec5d5032040fe494cc032094448c9c05c8557665449R188-R190

Thank you for checking. I'll add validation logic and applied the changes to the specified code that you mentioned.

core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java

minwoox · 2024-05-16T12:06:08Z

core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java

+import com.linecorp.armeria.server.HttpServiceOptions;
+
+/**
+ * An annotation used to configure {@link HttpServiceOptions} of a {@link HttpService}.


nit:

Suggested change

* An annotation used to configure {@link HttpServiceOptions} of a {@link HttpService}.

* An annotation used to configure {@link HttpServiceOptions} of an {@link HttpService}.

I've also changed a to an in the WrappingTransientHttpService's doc.

minwoox · 2024-05-16T12:07:24Z

core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java

+ * An annotation used to configure {@link HttpServiceOptions} of a {@link HttpService}.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ ElementType.METHOD })


What do you think of also adding ElementType.TYPE?

Sure. I'll add support for ElementType.TYPE 🙇

minwoox · 2024-05-16T12:26:23Z

core/src/main/java/com/linecorp/armeria/internal/server/websocket/DefaultWebSocketService.java

@@ -397,4 +406,9 @@ public HttpResponse encode(ServiceRequestContext ctx, WebSocket out) {
 public WebSocketProtocolHandler protocolHandler() {
 return this;
 }
+
+ @Override
+ public HttpServiceOptions options() {


We also need to add this to GraphqlWebSocketService.

If a WebSocketService fails to upgrade here:

armeria/core/src/main/java/com/linecorp/armeria/server/websocket/WebSocketService.java

Line 54 in 3963f2a

return upgradeResult.fallbackResponse();

we need to set the HttpServiceOptions from the fallbackService.

@Nullable HttpService fallbackService(); @Override default HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception { final WebSocketUpgradeResult upgradeResult = protocolHandler().upgrade(ctx, req); if (!upgradeResult.isSuccess()) { if (fallbackService() != null) { ctx.setRequestTimeoutMillis(fallbackService().options().requestTimeoutMillis()); ... } return upgradeResult.fallbackResponse(); } final WebSocket in = protocolHandler().decode(ctx, req); final WebSocket out = serve(ctx, in); return protocolHandler().encode(ctx, out); }

What do you think of setting the HttpServiceOptions in the DefaultWebSocketService#failOrFallback method like below?

private HttpResponse failOrFallback(ServiceRequestContext ctx, HttpRequest req, Supplier<HttpResponse> invalidResponse) throws Exception { if (fallbackService != null) { final HttpServiceOptions options = fallbackService.options(); if (options.requestTimeoutMillis() > 0) { ctx.setRequestTimeoutMillis(options.requestTimeoutMillis()); } ... return fallbackService.serve(ctx, req); } else { return invalidResponse.get(); } }

The reason for above suggestion is because we can set HttpServiceOptions while we are creating a fallback response. If we set HttpServiceOption in WebSocketService, handling fallback logic is separated into two parts

final WebSocketUpgradeResult upgradeResult = protocolHandler().upgrade(ctx, req); : handles upgrade and create fallback response

if (fallbackService() != null) { ctx.setRequestTimeoutMillis(fallbackService().options().requestTimeoutMillis()); ... }
=> sets HttpServiceOptions for the fallbackResponse

What do you think of setting the HttpServiceOptions in the DefaultWebSocketService#failOrFallback method like below?

Oops, I missed it. Yeah we should set it in the location. 😉

Co-authored-by: minux <[email protected]>

minwoox

I think we also need to update RouteDecoratingService.

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 7, 2024 12:41

seonWKim marked this pull request as draft April 7, 2024 22:38

ikhoon reviewed Apr 11, 2024

View reviewed changes

seonWKim marked this pull request as ready for review April 13, 2024 10:04

seonWKim force-pushed the feature/ISSUE-5071-add-ServiceOptions-to-Service branch 2 times, most recently from 2cbba00 to 81e8d09 Compare April 14, 2024 05:12

Add HttpServiceOptions and HttpServiceOption

44f1946

seonWKim force-pushed the feature/ISSUE-5071-add-ServiceOptions-to-Service branch from 81e8d09 to 44f1946 Compare April 14, 2024 05:17

Rename

277ebaf

- OPTIONS -> DEFAULT OPTIONS - WEBSOCKET_DEFAULT -> WEB_SOCKET_DEFAULT_OPTIONS

ikhoon reviewed Apr 22, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java Outdated Show resolved Hide resolved

Configure below options in the constructor of ServiceConfigBuilder

d65a265

- requestTimeoutMillis - maxRequestLength - requestAutoAbortDelayMillis

seonWKim force-pushed the feature/ISSUE-5071-add-ServiceOptions-to-Service branch from 9f73765 to d65a265 Compare April 27, 2024 01:03

Add TODO

44fbf28

ikhoon reviewed May 2, 2024

View reviewed changes

ikhoon reviewed May 3, 2024

View reviewed changes

seonWKim added 4 commits May 11, 2024 11:01

Apply comments

da9c92d

- Move DEFAULT_OPTIONS of webSocket under DefaultWebSocketService - Add test to verify priority of the options

Merge branch 'main' into feature/ISSUE-5071-add-ServiceOptions-to-Ser…

993847f

…vice

Nit

9748985

Nit

89d318b

minwoox added the new feature label May 16, 2024

minwoox added this to the 1.29.0 milestone May 16, 2024

minwoox reviewed May 16, 2024

View reviewed changes

seonWKim and others added 2 commits May 17, 2024 09:01

Apply suggestions from code review

93a37e9

Co-authored-by: minux <[email protected]>

Apply comments

a15ef81

minwoox reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ServiceOptions` and `@ServiceOption` #5574

Add `ServiceOptions` and `@ServiceOption` #5574

seonWKim commented Apr 7, 2024

seonWKim commented Apr 7, 2024

seonWKim commented Apr 7, 2024 •

edited

github-actions bot commented Apr 7, 2024 •

edited

ikhoon commented Apr 11, 2024

ikhoon commented Apr 11, 2024

seonWKim commented Apr 13, 2024

codecov bot commented Apr 13, 2024 •

edited

seonWKim commented Apr 14, 2024

ikhoon May 2, 2024

seonWKim May 11, 2024

ikhoon May 3, 2024

seonWKim May 11, 2024 •

edited

minwoox May 16, 2024

seonWKim May 17, 2024 •

edited

minwoox May 16, 2024

seonWKim May 17, 2024 •

edited

minwoox May 16, 2024

seonWKim May 17, 2024

minwoox May 16, 2024

seonWKim May 17, 2024 •

edited

minwoox May 16, 2024

seonWKim May 18, 2024 •

edited

minwoox May 20, 2024

minwoox left a comment

	* An annotation used to configure {@link HttpServiceOptions} of a {@link HttpService}.
	* An annotation used to configure {@link HttpServiceOptions} of an {@link HttpService}.

Add ServiceOptions and @ServiceOption #5574

Are you sure you want to change the base?

Add ServiceOptions and @ServiceOption #5574

Conversation

seonWKim commented Apr 7, 2024

seonWKim commented Apr 7, 2024

seonWKim commented Apr 7, 2024 • edited

github-actions bot commented Apr 7, 2024 • edited

🔍 Build Scan® (commit: a15ef81)

ikhoon commented Apr 11, 2024

ikhoon commented Apr 11, 2024

seonWKim commented Apr 13, 2024

codecov bot commented Apr 13, 2024 • edited

Codecov Report

seonWKim commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Add `ServiceOptions` and `@ServiceOption` #5574

Add `ServiceOptions` and `@ServiceOption` #5574

seonWKim commented Apr 7, 2024 •

edited

github-actions bot commented Apr 7, 2024 •

edited

🔍 Build Scan® (commit: `a15ef81`)

codecov bot commented Apr 13, 2024 •

edited

seonWKim May 11, 2024 •

edited

seonWKim May 17, 2024 •

edited

seonWKim May 17, 2024 •

edited

seonWKim May 17, 2024 •

edited

seonWKim May 18, 2024 •

edited