Do not create and cache handler for fallbacks. These handlers require access to the current invocation context (for fallback method parameters) and caching them fails when that changes. Handlers are now cached for all FT annotations except fallbacks. New test added to verify the changes. (helidon-io#2546)

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

Author: spericas

microprofile/fault-tolerance/src/main/java/io/helidon/microprofile/faulttolerance/MethodInvoker.java

@@ -161,17 +161,22 @@ class MethodInvoker implements FtSupplier<Object> {
      * FT handler created for the method.
      */
     private static class MethodState {
-        private FtHandlerTyped<Object> handler;
         private Retry retry;
         private Bulkhead bulkhead;
         private CircuitBreaker breaker;
+        private Timeout timeout;
         private State lastBreakerState;
         private long breakerTimerOpen;
         private long breakerTimerClosed;
         private long breakerTimerHalfOpen;
         private long startNanos;
     }
 
+    /**
+     * FT handler for this invoker.
+     */
+    private FtHandlerTyped<Object> handler;
+
     /**
      * A key used to lookup {@code MethodState} instances, which include FT handlers.
      * A class loader is necessary to support multiple applications as seen in the TCKs.
@@ -302,10 +307,13 @@ public boolean cancel(boolean mayInterruptIfRunning) {
                 methodState.breakerTimerHalfOpen = 0L;
                 methodState.startNanos = System.nanoTime();
             }
-            methodState.handler = createMethodHandler(methodState);
+            initMethodHandler(methodState);
             return methodState;
         });
 
+        // Create a new method handler to ensure correct context in fallback
+        handler = createMethodHandler(methodState);
+
         // Gather information about current request scope if active
         try {
             requestController = CDI.current().select(RequestContextController.class).get();
@@ -380,7 +388,7 @@ public Object get() throws Throwable {
         Supplier<Single<?>> supplier = () -> {
             try {
                 return Contexts.runInContextWithThrow(helidonContext,
-                        () -> methodState.handler.invoke(toCompletionStageSupplier(context::proceed)));
+                        () -> handler.invoke(toCompletionStageSupplier(context::proceed)));
             } catch (Exception e) {
                 return Single.error(e);
             }
@@ -496,38 +504,27 @@ private FtSupplier<Object> requestContextSupplier(FtSupplier<Object> supplier) {
     }
 
     /**
-     * Creates a FT handler for an invocation by inspecting annotations. Handlers
-     * are composed as follows:
-     *
-     *  fallback(retry(circuitbreaker(timeout(bulkhead(method)))))
-     *
-     * Note that timeout includes the time an invocation may be queued in a
-     * bulkhead, so it needs to be before the bulkhead call.
+     * Initializes method state by creating handlers for all FT annotations
+     * except fallbacks. A fallback can reference the current invocation context
+     * (via fallback method parameters) and cannot be cached.
      *
      * @param methodState State related to this invocation's method.
      */
-    private FtHandlerTyped<Object> createMethodHandler(MethodState methodState) {
-        FaultTolerance.TypedBuilder<Object> builder = FaultTolerance.typedBuilder();
-
-        // Create and add bulkhead
+    private void initMethodHandler(MethodState methodState) {
         if (introspector.hasBulkhead()) {
             methodState.bulkhead = Bulkhead.builder()
                     .limit(introspector.getBulkhead().value())
                     .queueLength(introspector.isAsynchronous() ? introspector.getBulkhead().waitingTaskQueue() : 0)
                     .build();
-            builder.addBulkhead(methodState.bulkhead);
         }
 
-        // Create and add timeout handler
         if (introspector.hasTimeout()) {
-            Timeout timeout = Timeout.builder()
+            methodState.timeout = Timeout.builder()
                     .timeout(Duration.of(introspector.getTimeout().value(), introspector.getTimeout().unit()))
                     .currentThread(!introspector.isAsynchronous())
                     .build();
-            builder.addTimeout(timeout);
         }
 
-        // Create and add circuit breaker
         if (introspector.hasCircuitBreaker()) {
             methodState.breaker = CircuitBreaker.builder()
                     .delay(Duration.of(introspector.getCircuitBreaker().delay(),
@@ -538,29 +535,55 @@ private FtHandlerTyped<Object> createMethodHandler(MethodState methodState) {
                     .applyOn(mapTypes(introspector.getCircuitBreaker().failOn()))
                     .skipOn(mapTypes(introspector.getCircuitBreaker().skipOn()))
                     .build();
-            builder.addBreaker(methodState.breaker);
         }
 
-        // Create and add retry handler
         if (introspector.hasRetry()) {
-            Retry retry = Retry.builder()
+            methodState.retry = Retry.builder()
                     .retryPolicy(Retry.JitterRetryPolicy.builder()
                             .calls(introspector.getRetry().maxRetries() + 1)
                             .delay(Duration.of(introspector.getRetry().delay(),
-                                               introspector.getRetry().delayUnit()))
+                                    introspector.getRetry().delayUnit()))
                             .jitter(Duration.of(introspector.getRetry().jitter(),
-                                                introspector.getRetry().jitterDelayUnit()))
+                                    introspector.getRetry().jitterDelayUnit()))
                             .build())
                     .overallTimeout(Duration.of(introspector.getRetry().maxDuration(),
-                                                introspector.getRetry().durationUnit()))
+                            introspector.getRetry().durationUnit()))
                     .applyOn(mapTypes(introspector.getRetry().retryOn()))
                     .skipOn(mapTypes(introspector.getRetry().abortOn()))
                     .build();
-            builder.addRetry(retry);
-            methodState.retry = retry;      // keep reference to Retry
+        }
+    }
+
+    /**
+     * Creates a FT handler for this invocation. Handlers are composed as follows:
+     *
+     *  fallback(retry(circuitbreaker(timeout(bulkhead(method)))))
+     *
+     * Uses the cached handlers defined in the method state for this invocation's
+     * method, except for fallback.
+     *
+     * @param methodState State related to this invocation's method.
+     */
+    private FtHandlerTyped<Object> createMethodHandler(MethodState methodState) {
+        FaultTolerance.TypedBuilder<Object> builder = FaultTolerance.typedBuilder();
+
+        if (methodState.bulkhead != null) {
+            builder.addBulkhead(methodState.bulkhead);
+        }
+
+        if (methodState.timeout != null) {
+            builder.addTimeout(methodState.timeout);
+        }
+
+        if (methodState.breaker != null) {
+            builder.addBreaker(methodState.breaker);
+        }
+
+        if (methodState.retry != null) {
+            builder.addRetry(methodState.retry);
         }
 
-        // Create and add fallback handler
+        // Create and add fallback handler for this invocation
         if (introspector.hasFallback()) {
             Fallback<Object> fallback = Fallback.builder()
                     .fallback(throwable -> {

tests/functional/request-scope/src/main/java/io/helidon/tests/functional/requestscope/MyResource.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.helidon.tests.functional.requestscope;
+
+import javax.enterprise.context.RequestScoped;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+
+@RequestScoped
+@Path("/test3")
+public class MyResource {
+
+    @Inject
+    SomeService3 someService3;
+
+    @GET
+    public String getTestResource(@QueryParam("param1") String param1) {
+        try {
+            return someService3.test(param1);
+        } catch (Exception e) {
+            System.out.println(e.getMessage());
+            throw new WebApplicationException(e, Response.Status.INTERNAL_SERVER_ERROR);
+        }
+    }
+
+}

tests/functional/request-scope/src/main/java/io/helidon/tests/functional/requestscope/SomeService3.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.helidon.tests.functional.requestscope;
+
+import javax.enterprise.context.ApplicationScoped;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.eclipse.microprofile.faulttolerance.Fallback;
+
+@ApplicationScoped
+public class SomeService3 {
+
+    private AtomicLong counter = new AtomicLong(0);
+
+    @Fallback(fallbackMethod = "testFallback")
+    public String test(String testParam) throws Exception {
+        maybeFail();
+        return testParam;
+    }
+
+    public String testFallback(String testParam) throws Exception {
+        return testParam;
+    }
+
+    private void maybeFail() {
+        final Long invocationNumber = counter.getAndIncrement();
+        if (invocationNumber % 4 > 1) { // alternate 2 successful and 2 failing invocations
+            throw new RuntimeException("Service failed.");
+        }
+    }
+}

tests/functional/request-scope/src/test/java/io/helidon/tests/functional/requestscope/TenantTest.java

@@ -51,4 +51,19 @@ public void test2() {
             assertThat(r.getStatus(), is(HttpResponseStatus.OK.code()));
         }
     }
+
+    @Test
+    public void test3() {
+        Response r;
+        for (int i = 0; i < 3; i++) {
+            String paramValue = Integer.toString(i);
+            r = baseTarget.path("test3")
+                    .queryParam("param1", paramValue)
+                    .request()
+                    .get();
+            assertThat(r.getStatus(), is(HttpResponseStatus.OK.code()));
+            String entityValue = r.readEntity(String.class);
+            assertThat(entityValue, is(paramValue));
+        }
+    }
 }