Introduce AsyncContext test fixture for service filters

[tkountis]

Motivation:
Have a test-fixture to verify async context visibility when requests go through our filters.

Modifications:
A new utility class that given a filter verifies that async context is properly propagated and shared across operators.
Fixed operators that weren't correctly handling that.

Result:
Enhanced testing and fixed broken filters.

[tkountis]

The client side will be on a follow up PR

[tkountis]

Build failure due to #1507

[bondolo]

It is nice to see that it found problems to fix. Perhaps submit the fixes separately?

[tkountis]

Thanks for the review @bondolo. I reckon a single pr with all the places code is broken for that single unit test is ok, but we may need to list the fixed filters in the changelog. Will keep that in mind for next release.
@idelpivnitskiy thoughts?

[idelpivnitskiy]

Great utility! Happy to see a consistent approach for all filters ❤️

Consider also adding tests for:

• RequestTargetDecoderHttpServiceFilter
• RequestTargetEncoderHttpServiceFilter
• GrpcServiceBuilder#CatchAllHttpServiceFilter (you can make it pkg-private for testing)

P.S.

I reckon a single pr with all the places code is broken for that single unit test is ok, but we may need to list the fixed filters in the changelog.

+1, better to have a fix + reproducer in one PR.
+1 for listing all affected filters in release notes and the current commit/PR message.

[idelpivnitskiy]

Thanks!

[idelpivnitskiy]

Since I was playing around, here is my patch with local changes:

Index: servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TracingHttpServiceFilterTest.java
===================================================================
diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TracingHttpServiceFilterTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TracingHttpServiceFilterTest.java
deleted file mode 100644
--- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TracingHttpServiceFilterTest.java	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ /dev/null	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
@@ -1,33 +0,0 @@
-/*
- * Copyright © 2021 Apple Inc. and the ServiceTalk project authors
- *
- * 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.servicetalk.http.netty;
-
-import io.servicetalk.opentracing.http.TracingHttpServiceFilter;
-import io.servicetalk.opentracing.inmemory.DefaultInMemoryTracer;
-
-import org.junit.Test;
-
-import static io.servicetalk.http.netty.AsyncContextHttpFilterVerifier.verifyServerFilterAsyncContextVisibility;
-import static io.servicetalk.opentracing.asynccontext.AsyncContextInMemoryScopeManager.SCOPE_MANAGER;
-
-public class TracingHttpServiceFilterTest {
-
-    @Test
-    public void verifyAsyncContext() throws Exception {
-        final DefaultInMemoryTracer tracer = new DefaultInMemoryTracer.Builder(SCOPE_MANAGER).build();
-        verifyServerFilterAsyncContextVisibility(new TracingHttpServiceFilter(tracer, "testServer"));
-    }
-}
Index: servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/auth/BasicAuthHttpServiceFilter.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/auth/BasicAuthHttpServiceFilter.java b/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/auth/BasicAuthHttpServiceFilter.java
--- a/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/auth/BasicAuthHttpServiceFilter.java	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ b/servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/auth/BasicAuthHttpServiceFilter.java	(date 1620341684081)
@@ -321,7 +321,7 @@
                     userIdAndPassword.substring(colonIdx + 1);
 
             return config.credentialsVerifier.apply(userId, password)
-                    .flatMap(userInfo -> onAuthenticated(ctx, request, factory, userInfo))
+                    .flatMap(userInfo -> onAuthenticated(ctx, request, factory, userInfo).subscribeShareContext())
                     .onErrorResume(t -> {
                         if (t instanceof AuthenticationException) {
                             return onAccessDenied(request, factory);
@@ -359,7 +359,7 @@
             if (config.userInfoKey != null) {
                 AsyncContext.put(config.userInfoKey, userInfo);
             }
-            return delegate().handle(ctx, request, factory).subscribeShareContext();
+            return delegate().handle(ctx, request, factory);
         }
     }
 }
Index: servicetalk-opentracing-http/build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-opentracing-http/build.gradle b/servicetalk-opentracing-http/build.gradle
--- a/servicetalk-opentracing-http/build.gradle	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ b/servicetalk-opentracing-http/build.gradle	(date 1620341380880)
@@ -32,6 +32,7 @@
   testImplementation testFixtures(project(":servicetalk-concurrent-internal"))
   testImplementation testFixtures(project(":servicetalk-transport-netty-internal"))
   testImplementation testFixtures(project(":servicetalk-log4j2-mdc-utils"))
+  testImplementation testFixtures(project(":servicetalk-http-netty"))
   testImplementation project(":servicetalk-buffer-netty")
   testImplementation project(":servicetalk-data-jackson")
   testImplementation project(":servicetalk-http-netty")
Index: servicetalk-opentracing-http/src/test/java/io/servicetalk/opentracing/http/TracingHttpServiceFilterTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-opentracing-http/src/test/java/io/servicetalk/opentracing/http/TracingHttpServiceFilterTest.java b/servicetalk-opentracing-http/src/test/java/io/servicetalk/opentracing/http/TracingHttpServiceFilterTest.java
--- a/servicetalk-opentracing-http/src/test/java/io/servicetalk/opentracing/http/TracingHttpServiceFilterTest.java	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ b/servicetalk-opentracing-http/src/test/java/io/servicetalk/opentracing/http/TracingHttpServiceFilterTest.java	(date 1620341313469)
@@ -59,6 +59,7 @@
 import static io.servicetalk.http.api.HttpResponseStatus.INTERNAL_SERVER_ERROR;
 import static io.servicetalk.http.api.HttpSerializationProviders.jsonSerializer;
 import static io.servicetalk.http.api.HttpSerializationProviders.textSerializer;
+import static io.servicetalk.http.netty.AsyncContextHttpFilterVerifier.verifyServerFilterAsyncContextVisibility;
 import static io.servicetalk.http.netty.HttpClients.forSingleAddress;
 import static io.servicetalk.log4j2.mdc.utils.LoggerStringWriter.stableAccumulated;
 import static io.servicetalk.opentracing.asynccontext.AsyncContextInMemoryScopeManager.SCOPE_MANAGER;
@@ -230,6 +231,12 @@
         }
     }
 
+    @Test
+    public void verifyAsyncContext() throws Exception {
+        final DefaultInMemoryTracer tracer = new DefaultInMemoryTracer.Builder(SCOPE_MANAGER).build();
+        verifyServerFilterAsyncContextVisibility(new TracingHttpServiceFilter(tracer, "testServer"));
+    }
+
     private static final class TestTracingLoggerFilter implements StreamingHttpServiceFilterFactory {
         private final String[] logLinePrefix;
 
Index: servicetalk-http-netty/build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-http-netty/build.gradle b/servicetalk-http-netty/build.gradle
--- a/servicetalk-http-netty/build.gradle	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ b/servicetalk-http-netty/build.gradle	(date 1620341488243)
@@ -43,7 +43,6 @@
   testImplementation testFixtures(project(":servicetalk-concurrent-internal"))
   testImplementation testFixtures(project(":servicetalk-http-api"))
   testImplementation testFixtures(project(":servicetalk-transport-netty-internal"))
-  testImplementation testFixtures(project(":servicetalk-http-netty"))
   testImplementation project(":servicetalk-concurrent-api-test")
   testImplementation project(":servicetalk-concurrent-test-internal")
   testImplementation project(":servicetalk-data-jackson")
@@ -51,8 +50,6 @@
   testImplementation project(":servicetalk-encoding-netty")
   testImplementation project(":servicetalk-test-resources")
   testImplementation project(":servicetalk-utils-internal")
-  testImplementation project(":servicetalk-opentracing-http")
-  testImplementation project(":servicetalk-opentracing-asynccontext")
   testImplementation "io.netty:netty-transport-native-unix-common:$nettyVersion"
   testImplementation "io.netty:netty-tcnative-boringssl-static:$tcnativeVersion"
   testImplementation "junit:junit:$junitVersion"
@@ -61,7 +58,5 @@
 
   testFixturesImplementation testFixtures(project(":servicetalk-transport-netty-internal"))
   testFixturesImplementation project(":servicetalk-http-utils")
-  testFixturesImplementation "junit:junit:$junitVersion"
   testFixturesImplementation "org.hamcrest:hamcrest-library:$hamcrestVersion"
-  testFixturesImplementation "org.mockito:mockito-core:$mockitoCoreVersion"
 }
Index: servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/AsyncContextHttpFilterVerifier.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/AsyncContextHttpFilterVerifier.java b/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/AsyncContextHttpFilterVerifier.java
--- a/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/AsyncContextHttpFilterVerifier.java	(revision 33ababe49e7bdea4663e40d136be1176c9430287)
+++ b/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/AsyncContextHttpFilterVerifier.java	(date 1620341563353)
@@ -49,7 +49,8 @@
 import static io.servicetalk.transport.netty.internal.AddressUtils.localAddress;
 import static io.servicetalk.transport.netty.internal.AddressUtils.serverHostAndPort;
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.junit.Assert.assertEquals;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
 
 /**
  * Utility verifiers for {@link StreamingHttpServiceFilterFactory} filters and their
@@ -89,8 +90,8 @@
                       .payloadBody(payload, textSerializer());
 
         final HttpResponse resp = client.request(request);
-        assertEquals(OK.code(), resp.status().code());
-        assertEquals(payload, resp.payloadBody(textDeserializer()));
+        assertThat(resp.status(), is(OK));
+        assertThat(resp.payloadBody(textDeserializer()), is(payload));
         assertEmpty(errors);
     }
 

[idelpivnitskiy]

Oups, found one thing:

java: Annotation processing is not supported for module cycles. Please ensure that all modules from cycle [servicetalk-opentracing-http,servicetalk-http-netty] are excluded from annotation processing

when tried to run any test from http-netty

[tkountis]

We should have a way to verify those during the build. I am running them with Idea (not Gradle) and they work and that's a false promise :(

[idelpivnitskiy]

Did you do ./gradlew idea before trying to run intellij idea test?

[tkountis]

I meant I am running them with the IntelliJ test runner so it doesn't complain.

[idelpivnitskiy]

It really depends on how your gradle project in imported in Intellij IDEA. I do not use built-in Intellij feature for gradle import. Instead, I run ./gradlew idea. It means that if I change anything in gradle files, I have to re-run this command manually in console to let Intellij see the changes.