diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcExecutionStrategy.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcExecutionStrategy.java index 9a0daf170c..3066c89fa9 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcExecutionStrategy.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcExecutionStrategy.java @@ -19,7 +19,7 @@ import static java.util.Objects.requireNonNull; -final class DefaultGrpcExecutionStrategy implements GrpcExecutionStrategy { +class DefaultGrpcExecutionStrategy implements GrpcExecutionStrategy { private final HttpExecutionStrategy delegate; diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcExecutionStrategies.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcExecutionStrategies.java index 14d46d7d50..084b86ba71 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcExecutionStrategies.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcExecutionStrategies.java @@ -25,10 +25,20 @@ public final class GrpcExecutionStrategies { private static final GrpcExecutionStrategy NEVER_OFFLOAD_STRATEGY = - new DefaultGrpcExecutionStrategy(HttpExecutionStrategies.offloadNever()); + new DefaultGrpcExecutionStrategy(HttpExecutionStrategies.offloadNever()) { + @Override + public HttpExecutionStrategy merge(final HttpExecutionStrategy other) { + return this; + } + }; private static final GrpcExecutionStrategy DEFAULT_GRPC_EXECUTION_STRATEGY = - new DefaultGrpcExecutionStrategy(HttpExecutionStrategies.defaultStrategy()); + new DefaultGrpcExecutionStrategy(HttpExecutionStrategies.defaultStrategy()) { + @Override + public HttpExecutionStrategy merge(final HttpExecutionStrategy other) { + return other; + } + }; private GrpcExecutionStrategies() { // No instances @@ -37,9 +47,8 @@ private GrpcExecutionStrategies() { /** * A special default {@link GrpcExecutionStrategy} that offloads all actions unless merged with another strategy * that requires less offloading. The intention of this strategy is to provide a safe default if no strategy is - * specified; it should not be returned by - * {@link HttpExecutionStrategyInfluencer#requiredOffloads()}, which should return - * {@link HttpExecutionStrategy#offloadNone()} or {@link HttpExecutionStrategy#offloadAll()} instead. + * specified; it should not be returned by {@link HttpExecutionStrategyInfluencer#requiredOffloads()} which should + * return the actual required offloads. * * @return Default {@link GrpcExecutionStrategy}. */ diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRouter.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRouter.java index c78c742ae1..62b6434f06 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRouter.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRouter.java @@ -92,6 +92,7 @@ import static io.servicetalk.grpc.api.GrpcUtils.setStatusOk; import static io.servicetalk.grpc.api.GrpcUtils.validateContentType; import static io.servicetalk.http.api.HttpApiConversions.toStreamingHttpService; +import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; import static io.servicetalk.http.api.HttpRequestMethod.POST; import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableMap; @@ -384,7 +385,7 @@ public Completable closeAsync() { public Completable closeAsyncGracefully() { return route.closeAsyncGracefully(); } - }, executionStrategy == null ? HttpExecutionStrategies.defaultStrategy() : executionStrategy), + }, executionStrategy == null ? defaultStrategy() : executionStrategy), () -> toStreaming(route), () -> toRequestStreamingRoute(route), () -> toResponseStreamingRoute(route), () -> route, route)), // We only assume duplication across blocking and async variant of the same API and not between @@ -458,9 +459,7 @@ public StreamingHttpService adaptor() { @Override public HttpExecutionStrategy serviceInvocationStrategy() { - return executionStrategy == null ? - HttpExecutionStrategies.defaultStrategy() : - executionStrategy; + return executionStrategy == null ? defaultStrategy() : executionStrategy; } }; }, () -> route, () -> toRequestStreamingRoute(route), () -> toResponseStreamingRoute(route), @@ -592,7 +591,7 @@ public void close() throws Exception { public void closeGracefully() throws Exception { route.closeGracefully(); } - }, executionStrategy == null ? HttpExecutionStrategies.defaultStrategy() : executionStrategy), + }, executionStrategy == null ? defaultStrategy() : executionStrategy), () -> toStreaming(route), () -> toRequestStreamingRoute(route), () -> toResponseStreamingRoute(route), () -> toRoute(route), route)), // We only assume duplication across blocking and async variant of the same API and not between @@ -662,7 +661,7 @@ public void close() throws Exception { public void closeGracefully() throws Exception { route.closeGracefully(); } - }, executionStrategy == null ? HttpExecutionStrategies.defaultStrategy() : executionStrategy), + }, executionStrategy == null ? defaultStrategy() : executionStrategy), () -> toStreaming(route), () -> toRequestStreamingRoute(route), () -> toResponseStreamingRoute(route), () -> toRoute(route), route)), // We only assume duplication across blocking and async variant of the same API and not between diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java index d071182922..96f7c596bc 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java @@ -16,6 +16,7 @@ package io.servicetalk.grpc.netty; import io.servicetalk.concurrent.BlockingIterator; +import io.servicetalk.concurrent.api.AsyncCloseable; import io.servicetalk.concurrent.api.Publisher; import io.servicetalk.concurrent.api.Single; import io.servicetalk.concurrent.test.internal.TestPublisherSubscriber; @@ -58,9 +59,11 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Objects; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.stream.Stream; import javax.annotation.Nullable; import static io.servicetalk.concurrent.api.AsyncCloseables.newCompositeCloseable; @@ -418,9 +421,8 @@ private void setupForBlockingServiceWritesThenThrows(final BlockingTesterService } static Collection data() { - GrpcExecutionStrategy noopStrategy = offloadNever(); GrpcExecutionStrategy[] strategies = - new GrpcExecutionStrategy[]{noopStrategy, defaultStrategy()}; + new GrpcExecutionStrategy[]{offloadNever(), defaultStrategy()}; List data = new ArrayList<>(strategies.length * 2 * TestMode.values().length); for (GrpcExecutionStrategy serverStrategy : strategies) { for (GrpcExecutionStrategy clientStrategy : strategies) { @@ -437,9 +439,14 @@ static Collection data() { @AfterEach void tearDown() throws Exception { try { - blockingClient.close(); + if (null != blockingClient) { + blockingClient.close(); + } } finally { - newCompositeCloseable().appendAll(client, serverContext).close(); + newCompositeCloseable().appendAll(Stream.of(client, serverContext) + .filter(Objects::nonNull) + .toArray(AsyncCloseable[]::new) + ).close(); } } diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/ConnectAndHttpExecutionStrategy.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/ConnectAndHttpExecutionStrategy.java index 16001b312d..78da2c5a49 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/ConnectAndHttpExecutionStrategy.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/ConnectAndHttpExecutionStrategy.java @@ -20,6 +20,8 @@ import java.util.Objects; +import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; + /** * Combines a {@link ConnectExecutionStrategy} and an {@link HttpExecutionStrategy}. */ @@ -122,7 +124,8 @@ private ConnectAndHttpExecutionStrategy merge(final ConnectExecutionStrategy oth @Override public ConnectAndHttpExecutionStrategy merge(final HttpExecutionStrategy other) { - HttpExecutionStrategy merged = httpStrategy.merge(other); + HttpExecutionStrategy merged = defaultStrategy() == httpStrategy ? + other : defaultStrategy() == other ? httpStrategy : httpStrategy.merge(other); return merged == httpStrategy ? this : new ConnectAndHttpExecutionStrategy(connectStrategy, merged); } @@ -164,7 +167,7 @@ public static ConnectAndHttpExecutionStrategy from(ExecutionStrategy executionSt return executionStrategy instanceof ConnectAndHttpExecutionStrategy ? (ConnectAndHttpExecutionStrategy) executionStrategy : new ConnectAndHttpExecutionStrategy( - ConnectExecutionStrategy.offloadNone(), HttpExecutionStrategies.defaultStrategy()) + ConnectExecutionStrategy.offloadNone(), defaultStrategy()) .merge(executionStrategy); } } diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpExecutionStrategy.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpExecutionStrategy.java index b4a31aff2f..70b62119fc 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpExecutionStrategy.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpExecutionStrategy.java @@ -140,7 +140,6 @@ public HttpExecutionStrategy merge(final HttpExecutionStrategy other) { } // merge the offload flags - byte otherOffloads = generateOffloadsFlag(other); return offloads == (offloads | otherOffloads) ? diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategies.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategies.java index 7025406b46..6498fdb613 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategies.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategies.java @@ -36,11 +36,10 @@ private HttpExecutionStrategies() { } /** - * A special default {@link HttpExecutionStrategy} that offloads all actions unless merged with another strategy - * that requires less offloading. The intention of this strategy is to provide a safe default if no strategy is - * specified; it should not be returned by - * {@link HttpExecutionStrategyInfluencer#requiredOffloads()}, which should return {@link #offloadNone()} or - * {@link #offloadAll()} instead. + * A special default {@link HttpExecutionStrategy} that provides safe default offloading of actions; the offloading + * used is unspecified and dependent upon the usage situation. It may not be merged with other strategies because + * the resulting strategy would lose the defaulting behavior. As an additional safety measure all offload query + * methods will return true. * * @return Default {@link HttpExecutionStrategy}. */ diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategyInfluencer.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategyInfluencer.java index 5b497ada73..b2f6fc8582 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategyInfluencer.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpExecutionStrategyInfluencer.java @@ -20,7 +20,6 @@ import java.lang.reflect.Method; import static io.servicetalk.http.api.DefaultStreamingStrategyInfluencer.DEFAULT_STREAMING_STRATEGY_INFLUENCER; -import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; import static io.servicetalk.http.api.HttpExecutionStrategies.offloadAll; import static io.servicetalk.http.api.HttpExecutionStrategies.offloadNone; @@ -33,6 +32,9 @@ public interface HttpExecutionStrategyInfluencer extends ExecutionStrategyInflue * Optionally modify the passed {@link HttpExecutionStrategy} to a new {@link HttpExecutionStrategy} that suits * this {@link HttpExecutionStrategyInfluencer}. * + *

Implementations should not return {@link HttpExecutionStrategies#defaultStrategy()} unless it was also + * provided as input.

+ * * @param strategy {@link HttpExecutionStrategy} to influence. * @return {@link HttpExecutionStrategy} that suits this {@link HttpExecutionStrategyInfluencer} * @deprecated Implement {@link #requiredOffloads()} instead. @@ -56,13 +58,13 @@ default HttpExecutionStrategy influenceStrategy(HttpExecutionStrategy strategy) * *

The provided default implementation requests offloading of all operations. Implementations that require no * offloading should be careful to return {@link HttpExecutionStrategies#offloadNone()} rather than - * {@link HttpExecutionStrategies#offloadNever()}. + * {@link HttpExecutionStrategies#offloadNever()}. Implementations should avoid returning + * {@link HttpExecutionStrategies#defaultStrategy()}, instead returning the strategy they require or + * {@link HttpExecutionStrategies#offloadAll()} if offloading for all paths is required (safe default). */ @Override default HttpExecutionStrategy requiredOffloads() { - // safe default--implementations are expected to override - HttpExecutionStrategy result = influenceStrategy(defaultStrategy()); - return defaultStrategy() == result ? offloadNone() : result; + return influenceStrategy(offloadNone()); } /** diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/SpecialHttpExecutionStrategy.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/SpecialHttpExecutionStrategy.java index 0e32b9fe6b..b5a61b92f3 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/SpecialHttpExecutionStrategy.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/SpecialHttpExecutionStrategy.java @@ -15,6 +15,9 @@ */ package io.servicetalk.http.api; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * Package private special purpose implementation for {@link HttpExecutionStrategy} to be used across programming model * adapters, should not be made public. Provides a special execution strategy that overrides offloading behavior. @@ -22,12 +25,10 @@ * @see DefaultHttpExecutionStrategy */ enum SpecialHttpExecutionStrategy implements HttpExecutionStrategy { - /** * Enforces no offloading and maintains this even when merged. */ OFFLOAD_NEVER_STRATEGY { - @Override public boolean hasOffloads() { return false; @@ -74,6 +75,8 @@ public HttpExecutionStrategy merge(final HttpExecutionStrategy other) { */ DEFAULT_HTTP_EXECUTION_STRATEGY { + private volatile boolean mergeWarning; + @Override public boolean hasOffloads() { return true; @@ -112,7 +115,14 @@ public boolean isCloseOffloaded() { */ @Override public HttpExecutionStrategy merge(final HttpExecutionStrategy other) { + assert false : "merging defaultStrategy() with other strategies is deprecated"; + if (!mergeWarning) { + mergeWarning = true; + LOGGER.warn("merging defaultStrategy() with other strategies is deprecated"); + } return other; } - } + }; + + static final Logger LOGGER = LoggerFactory.getLogger(SpecialHttpExecutionStrategy.class); } diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToBlockingStreamingHttpClient.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToBlockingStreamingHttpClient.java index 8162a224b0..a55b971893 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToBlockingStreamingHttpClient.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToBlockingStreamingHttpClient.java @@ -32,8 +32,7 @@ final class StreamingHttpClientToBlockingStreamingHttpClient implements Blocking StreamingHttpClientToBlockingStreamingHttpClient(final StreamingHttpClient client, final HttpExecutionStrategy strategy) { - this.strategy = defaultStrategy() == strategy ? - DEFAULT_BLOCKING_STREAMING_CONNECTION_STRATEGY : strategy; + this.strategy = defaultStrategy() == strategy ? DEFAULT_BLOCKING_STREAMING_CONNECTION_STRATEGY : strategy; this.client = client; context = new DelegatingHttpExecutionContext(client.executionContext()) { @Override diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToHttpClient.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToHttpClient.java index a0762eb431..cb32c3fcd8 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToHttpClient.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpClientToHttpClient.java @@ -32,8 +32,7 @@ final class StreamingHttpClientToHttpClient implements HttpClient { private final HttpRequestResponseFactory reqRespFactory; StreamingHttpClientToHttpClient(final StreamingHttpClient client, final HttpExecutionStrategy strategy) { - this.strategy = defaultStrategy() == strategy ? - DEFAULT_CONNECTION_STRATEGY : strategy; + this.strategy = defaultStrategy() == strategy ? DEFAULT_CONNECTION_STRATEGY : strategy; this.client = client; context = new DelegatingHttpExecutionContext(client.executionContext()) { @Override diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategiesTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategiesTest.java index 45506c7fb1..d8346d14ae 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategiesTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategiesTest.java @@ -31,15 +31,6 @@ class HttpExecutionStrategiesTest { - @Test - void mergeDefaultStrategy() { - HttpExecutionStrategy strategy = customStrategyBuilder().offloadAll().build(); - HttpExecutionStrategy merged = strategy.merge(defaultStrategy()); - assertThat("merge returned defaultStrategy.", merged, sameInstance(strategy)); - merged = defaultStrategy().merge(strategy); - assertThat("merge returned defaultStrategy.", merged, sameInstance(strategy)); - } - @Test void defaultShouldOffloadAll() { HttpExecutionStrategy strategy = defaultStrategy(); diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategyTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategyTest.java index f93e80dc24..348789f880 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategyTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/HttpExecutionStrategyTest.java @@ -113,7 +113,6 @@ public HttpExecutionStrategy requiredOffloads() { RequiredOnly requiredOnly = new RequiredOnly(); assertThat(requiredOnly.requiredOffloads(), sameInstance(MAGIC_REQUIRED_STRATEGY)); - assertThat(requiredOnly.influenceStrategy(defaultStrategy()), sameInstance(MAGIC_REQUIRED_STRATEGY)); } private interface MyOwnDefaultRequiredOffloads extends HttpExecutionStrategyInfluencer { @@ -129,7 +128,6 @@ class RequiredDefault implements MyOwnDefaultRequiredOffloads { } RequiredDefault requiredOnly = new RequiredDefault(); assertThat(requiredOnly.requiredOffloads(), sameInstance(MAGIC_REQUIRED_STRATEGY)); - assertThat(requiredOnly.influenceStrategy(defaultStrategy()), sameInstance(MAGIC_REQUIRED_STRATEGY)); } @Test @@ -173,7 +171,6 @@ public HttpExecutionStrategy influenceStrategy(HttpExecutionStrategy strategy) { Influence influence = new Influence(); assertThat(influence.requiredOffloads(), is(offloadSend)); assertThat(influence.influenceStrategy(offloadNone()), is(offloadSend)); - assertThat(influence.influenceStrategy(defaultStrategy()), is(offloadSend)); assertThat(influence.influenceStrategy(offloadSend), is(offloadSend)); } diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/StrategyInfluencerChainBuilderTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/StrategyInfluencerChainBuilderTest.java index 4fa9a3cb72..7961ddbcc1 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/StrategyInfluencerChainBuilderTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/StrategyInfluencerChainBuilderTest.java @@ -23,6 +23,7 @@ import javax.annotation.Nonnull; import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; +import static io.servicetalk.http.api.HttpExecutionStrategies.offloadNone; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.sameInstance; import static org.mockito.ArgumentMatchers.any; @@ -43,7 +44,7 @@ void deepCopy() { HttpExecutionStrategyInfluencer influencer2 = newNoInfluenceInfluencer(); chain2.append(influencer2); - defaultStrategy().merge(chain1.build().requiredOffloads()); + offloadNone().merge(chain1.build().requiredOffloads()); verifyNoInteractions(influencer2); } @@ -84,7 +85,7 @@ void appendAndPrepend(boolean conditional) { chain.append(influencer3); } - defaultStrategy().merge(chain.build().requiredOffloads()); + offloadNone().merge(chain.build().requiredOffloads()); InOrder inOrder = inOrder(influencer1, influencer2, influencer3); inOrder.verify(influencer1).requiredOffloads(); diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ClientStrategyInfluencerChainBuilder.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ClientStrategyInfluencerChainBuilder.java index ba66e98fff..bb567970d5 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ClientStrategyInfluencerChainBuilder.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ClientStrategyInfluencerChainBuilder.java @@ -32,6 +32,7 @@ import javax.annotation.Nullable; import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; +import static io.servicetalk.http.api.HttpExecutionStrategies.offloadAll; import static io.servicetalk.http.api.HttpExecutionStrategies.offloadNever; import static io.servicetalk.http.api.HttpExecutionStrategies.offloadNone; @@ -68,49 +69,77 @@ void add(HttpLoadBalancerFactory lb) { private void add(String purpose, ExecutionStrategyInfluencer influencer, HttpExecutionStrategy strategy) { if (offloadNever() == strategy) { - LOGGER.warn("Ignoring illegal {} required strategy ({}) for {}", purpose, strategy, influencer); + LOGGER.warn("{}#requiredOffloads() returns offloadNever(), which is unexpected. " + + "offloadNone() should be used instead. " + + "Making automatic adjustment, consider updating the {}.", + influencer, purpose); strategy = offloadNone(); } - if (strategy.hasOffloads()) { - clientChain = null != clientChain ? clientChain.merge(strategy) : strategy; + if (defaultStrategy() == strategy) { + LOGGER.warn("{}#requiredOffloads() returns defaultStrategy(), which is unexpected. " + + "offloadAll() (safe default) or more appropriate custom strategy should be used instead." + + "Making automatic adjustment, consider updating the {}.", + influencer, purpose); + strategy = offloadAll(); } + clientChain = null != clientChain ? clientChain.merge(strategy) : strategy; } void add(ConnectionFactoryFilter connectionFactoryFilter) { ExecutionStrategy filterOffloads = connectionFactoryFilter.requiredOffloads(); if (offloadNever() == filterOffloads) { - LOGGER.warn("Ignoring illegal connection factory required strategy ({}) for {}", - filterOffloads, connectionFactoryFilter); + LOGGER.warn("{}#requiredOffloads() returns offloadNever(), which is unexpected. " + + "offloadNone() should be used instead. " + + "Making automatic adjustment, consider updating the filter.", + connectionFactoryFilter); filterOffloads = offloadNone(); } - if (filterOffloads.hasOffloads()) { - connFactoryChain = null != connFactoryChain ? - connFactoryChain.merge(filterOffloads) : ConnectAndHttpExecutionStrategy.from(filterOffloads); + if (defaultStrategy() == filterOffloads) { + LOGGER.warn("{}#requiredOffloads() returns defaultStrategy(), which is unexpected. " + + "offloadAll() (safe default) or more appropriate custom strategy should be used instead." + + "Making automatic adjustment, consider updating the filter.", + connectionFactoryFilter); + filterOffloads = offloadAll(); } + connFactoryChain = null != connFactoryChain ? + connFactoryChain.merge(filterOffloads) : ConnectAndHttpExecutionStrategy.from(filterOffloads); } void add(StreamingHttpConnectionFilterFactory connectionFilter) { HttpExecutionStrategy filterOffloads = connectionFilter.requiredOffloads(); if (offloadNever() == filterOffloads) { - LOGGER.warn("Ignoring illegal connection filter required strategy ({}) for {}", - filterOffloads, connectionFilter); + LOGGER.warn("{}#requiredOffloads() returns offloadNever(), which is unexpected. " + + "offloadNone() should be used instead. " + + "Making automatic adjustment, consider updating the filter.", + connectionFilter); filterOffloads = offloadNone(); } + if (defaultStrategy() == filterOffloads) { + LOGGER.warn("{}#requiredOffloads() returns defaultStrategy(), which is unexpected. " + + "offloadAll() (safe default) or more appropriate custom strategy should be used instead." + + "Making automatic adjustment, consider updating the filter.", + connectionFilter); + filterOffloads = offloadAll(); + } if (filterOffloads.hasOffloads()) { connFilterChain = null != connFilterChain ? connFilterChain.merge(filterOffloads) : filterOffloads; } } HttpExecutionStrategy buildForClient(HttpExecutionStrategy transportStrategy) { - HttpExecutionStrategy clientStrategy = - null != clientChain ? transportStrategy.merge(clientChain) : transportStrategy; + HttpExecutionStrategy chainStrategy = clientChain; if (null != connFilterChain) { - clientStrategy = clientStrategy.merge(connFilterChain); + chainStrategy = null != chainStrategy ? chainStrategy.merge(connFilterChain) : connFilterChain; } if (null != connFactoryChain) { - clientStrategy = clientStrategy.merge(HttpExecutionStrategy.from(buildForConnectionFactory())); + HttpExecutionStrategy connectionFactoryStrategy = HttpExecutionStrategy.from(buildForConnectionFactory()); + chainStrategy = null != chainStrategy ? + chainStrategy.merge(connectionFactoryStrategy) : connectionFactoryStrategy; } - return clientStrategy; + + return (null == chainStrategy || !chainStrategy.hasOffloads()) ? + transportStrategy : + defaultStrategy() == transportStrategy ? chainStrategy : chainStrategy.merge(transportStrategy); } ExecutionStrategy buildForConnectionFactory() { diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java index 61dc635c3a..0deffb9a0c 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java @@ -170,7 +170,6 @@ private void startServer() throws Exception { // differently. final HttpServerBuilder serverBuilder = HttpServers.forAddress(bindAddress) .executor(serverExecutor) - .executionStrategy(defaultStrategy()) .socketOption(StandardSocketOptions.SO_SNDBUF, 100) .protocols(protocol) .transportObserver(serverTransportObserver) diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientEffectiveStrategyTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientEffectiveStrategyTest.java index 5c9e04b1bc..8943e61a86 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientEffectiveStrategyTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientEffectiveStrategyTest.java @@ -93,35 +93,35 @@ class ClientEffectiveStrategyTest { null, // unspecified offloadNever(), offloadNone(), - defaultStrategy(), + defaultStrategy(), // should be same as unspecified HttpExecutionStrategies.customStrategyBuilder().offloadSend().build(), offloadAll(), }; private static final HttpExecutionStrategy[] FILTER_STRATEGIES = { null, // absent - offloadNever(), // treated as "offloadNoneStrategy" + offloadNever(), // treated as "offloadNone" offloadNone(), - defaultStrategy(), // treated as "offloadNoneStrategy" HttpExecutionStrategies.customStrategyBuilder().offloadSend().build(), + defaultStrategy(), // treated as "offloadAll" offloadAll(), }; private static final HttpExecutionStrategy[] LB_STRATEGIES = { null, // absent - offloadNever(), // treated as "offloadNoneStrategy" + offloadNever(), // treated as "offloadNone" offloadNone(), - defaultStrategy(), // treated as "offloadNoneStrategy" HttpExecutionStrategies.customStrategyBuilder().offloadSend().build(), + defaultStrategy(), // treated as "offloadAll" offloadAll(), }; private static final HttpExecutionStrategy[] CF_STRATEGIES = { null, // absent - offloadNever(), // treated as "offloadNoneStrategy" + offloadNever(), // treated as "offloadNone" offloadNone(), - defaultStrategy(), // treated as "offloadNoneStrategy" HttpExecutionStrategies.customStrategyBuilder().offloadSend().build(), + defaultStrategy(), // treated as "offloadAll" offloadAll(), }; @@ -234,33 +234,34 @@ public HttpExecutionStrategy requiredOffloads() { * Computes the base execution strategy that the client will use based on the selected builder strategy, filter * strategy, load balancer strategy, connection factory filter strategy. * - * @param builderStrategy strategy specified for client builder or null to use builder default. - * @param filterStrategy strategy specified for client stream filter to be added to client builder or null if no + * @param builder strategy specified for client builder or null to use builder default. + * @param filter strategy specified for client stream filter to be added to client builder or null if no * filter will be added. - * @param lbStrategy strategy specified for load balancer factory to be added to client builder or null if no + * @param lb strategy specified for load balancer factory to be added to client builder or null if no * load balancer will be added. - * @param cfStrategy strategy specified for connection filter factory to be added to client builder or null if no + * @param cf strategy specified for connection filter factory to be added to client builder or null if no * connection filter will be added. - * @return The str + * @return The strategy as computed */ - private HttpExecutionStrategy computeClientExecutionStrategy(@Nullable final HttpExecutionStrategy builderStrategy, - @Nullable final HttpExecutionStrategy filterStrategy, - @Nullable final HttpExecutionStrategy lbStrategy, - @Nullable final HttpExecutionStrategy cfStrategy) { - // null means assume default strategy which is, unsurprisingly, defaultStrategy() - HttpExecutionStrategy computed = null == builderStrategy ? - defaultStrategy() : builderStrategy; - // null means no filter, noOffloadsStrategy() is illegal and replaced. - computed = null == filterStrategy || offloadNone() == filterStrategy || offloadNever() == filterStrategy ? - computed : computed.merge(filterStrategy); - computed = null == lbStrategy || offloadNone() == lbStrategy || - defaultStrategy() == lbStrategy || offloadNever() == lbStrategy ? - computed : computed.merge(lbStrategy); - computed = null == cfStrategy || offloadNone() == cfStrategy || - defaultStrategy() == cfStrategy || offloadNever() == cfStrategy ? - computed : computed.merge(cfStrategy); - - return computed; + private HttpExecutionStrategy computeClientExecutionStrategy(@Nullable final HttpExecutionStrategy builder, + @Nullable final HttpExecutionStrategy filter, + @Nullable final HttpExecutionStrategy lb, + @Nullable final HttpExecutionStrategy cf) { + HttpExecutionStrategy chain = mergeStrategies(cf, mergeStrategies(lb, filter)); + + HttpExecutionStrategy merged = null == chain || !chain.hasOffloads() ? + null == builder ? defaultStrategy() : builder : + null == builder || defaultStrategy() == builder ? chain : + offloadNever() != builder ? mergeStrategies(builder, chain) : offloadNone(); + + return merged; + } + + private @Nullable HttpExecutionStrategy mergeStrategies(@Nullable HttpExecutionStrategy first, + @Nullable HttpExecutionStrategy second) { + first = offloadNever() != first ? defaultStrategy() != first ? first : offloadAll() : offloadNone(); + second = offloadNever() != second ? defaultStrategy() != second ? second : offloadAll() : offloadNone(); + return null == first ? second : null == second ? first : first.merge(second); } private String getResponse(ClientType clientType, StreamingHttpClient client) throws Exception { diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionCloseHeaderHandlingTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionCloseHeaderHandlingTest.java index 2fe26c374a..dc000ae006 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionCloseHeaderHandlingTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionCloseHeaderHandlingTest.java @@ -60,7 +60,6 @@ import static io.servicetalk.concurrent.api.Completable.completed; import static io.servicetalk.concurrent.api.Completable.never; import static io.servicetalk.concurrent.api.Publisher.from; -import static io.servicetalk.http.api.HttpExecutionStrategies.defaultStrategy; import static io.servicetalk.http.api.HttpHeaderNames.CONNECTION; import static io.servicetalk.http.api.HttpHeaderNames.CONTENT_LENGTH; import static io.servicetalk.http.api.HttpHeaderValues.CLOSE; @@ -132,7 +131,6 @@ void setUp(boolean useUds, boolean viaProxy, boolean awaitRequestPayload) throws HttpServers.forAddress(localAddress(0))) .ioExecutor(serverCtx.ioExecutor()) .executor(serverCtx.executor()) - .executionStrategy(defaultStrategy()) .enableWireLogging("servicetalk-tests-wire-logger", TRACE, Boolean.TRUE::booleanValue) .appendConnectionAcceptorFilter(original -> new DelegatingConnectionAcceptor(original) { @Override @@ -208,7 +206,6 @@ public Completable accept(final ConnectionContext context) { HttpClients.forResolvedAddress(serverContext.listenAddress())) .executor(clientCtx.executor()) .ioExecutor(clientCtx.ioExecutor()) - .executionStrategy(defaultStrategy()) .enableWireLogging("servicetalk-tests-wire-logger", TRACE, () -> true) .buildStreaming(); connection = client.reserveConnection(client.get("/")).toFuture().get(); diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java index 8d1539b246..d07a26f12c 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java @@ -68,7 +68,6 @@ void buildWithDefaults() throws Exception { StreamingHttpRequester newRequester = HttpClients.forMultiAddressUrl() .ioExecutor(CTX.ioExecutor()) .executor(CTX.executor()) - .executionStrategy(defaultStrategy()) .buildStreaming(); assertNotNull(newRequester); newRequester.closeAsync().toFuture().get(); @@ -90,7 +89,6 @@ void buildBlockingWithDefaults() throws Exception { BlockingStreamingHttpRequester newBlockingRequester = HttpClients.forMultiAddressUrl() .ioExecutor(CTX.ioExecutor()) .executor(CTX.executor()) - .executionStrategy(defaultStrategy()) .buildBlockingStreaming(); assertNotNull(newBlockingRequester); newBlockingRequester.close(); @@ -101,7 +99,6 @@ void buildBlockingAggregatedWithDefaults() throws Exception { BlockingHttpRequester newBlockingAggregatedRequester = HttpClients.forMultiAddressUrl() .ioExecutor(CTX.ioExecutor()) .executor(CTX.executor()) - .executionStrategy(defaultStrategy()) .buildBlocking(); assertNotNull(newBlockingAggregatedRequester); newBlockingAggregatedRequester.close(); @@ -114,7 +111,6 @@ void buildWithProvidedServiceDiscoverer() throws Exception { ServiceDiscovererEvent> mockedServiceDiscoverer = mock(ServiceDiscoverer.class); StreamingHttpRequester newRequester = HttpClients.forMultiAddressUrl(mockedServiceDiscoverer) .ioExecutor(CTX.ioExecutor()) - .executionStrategy(defaultStrategy()) .buildStreaming(); newRequester.closeAsync().toFuture().get(); verify(mockedServiceDiscoverer, never()).closeAsync(); diff --git a/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutFromRequest.java b/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutFromRequest.java index 1883c96fea..13c18965ed 100644 --- a/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutFromRequest.java +++ b/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutFromRequest.java @@ -29,7 +29,7 @@ /** * A function to determine the appropriate timeout to be used for a given {@link HttpRequestMetaData HTTP request}. * The result is a {@link Duration} which may be null if no timeout is to be applied. If the function blocks then - * {@link #influenceStrategy(HttpExecutionStrategy)} should alter the execution strategy as required. + * {@link #requiredOffloads()} should specify the execution strategy as required. * @deprecated In areas which require {@link TimeoutFromRequest} use variants that accept * {@link java.util.function.BiFunction}<{@link HttpRequestMetaData}, {@link TimeSource}, {@link Duration}>. * E.g.: