Remove special defaultStrategy() merge method

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;
 

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}.
      */

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

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<Arguments> data() {
-        GrpcExecutionStrategy noopStrategy = offloadNever();
         GrpcExecutionStrategy[] strategies =
-                new GrpcExecutionStrategy[]{noopStrategy, defaultStrategy()};
+                new GrpcExecutionStrategy[]{offloadNever(), defaultStrategy()};
         List<Arguments> data = new ArrayList<>(strategies.length * 2 * TestMode.values().length);
         for (GrpcExecutionStrategy serverStrategy : strategies) {
             for (GrpcExecutionStrategy clientStrategy : strategies) {
@@ -437,9 +439,14 @@ static Collection<Arguments> 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();
         }
     }
 

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);
     }
 }

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) ?

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}.
      */

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}.
      *
+     * <p>Implementations should not return {@link HttpExecutionStrategies#defaultStrategy()} unless it was also
+     * provided as input.</p>
+     *
      * @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)
      *
      * <p>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());
     }
 
     /**

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SpecialHttpExecutionStrategy.java

@@ -15,19 +15,20 @@
  */
 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.
  *
  * @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);
 }

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

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

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();

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));
     }
 

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();