minimal fix

jrhee17 · Nov 18, 2024 · 4aa3c17 · 4aa3c17
1 parent 4bc956f
commit 4aa3c17
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 4 deletions.
diff --git a/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java b/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java
@@ -1104,12 +1104,11 @@ private void endRequest0(@Nullable Throwable requestCause, long requestEndTimeNa
             }
         }
 
-        // Set names if request content is not deferred or it was deferred but has been set
-        // before the request completion.
-        if (!hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT) ||
-            isAvailable(RequestLogProperty.REQUEST_CONTENT)) {
+        // Set names if request content is not deferred
+        if (!hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT)) {
             setNamesIfAbsent();
         }
+
         this.requestEndTimeNanos = requestEndTimeNanos;
 
         if (requestCause instanceof HttpStatusException || requestCause instanceof HttpResponseException) {
@@ -1119,6 +1118,12 @@ private void endRequest0(@Nullable Throwable requestCause, long requestEndTimeNa
             this.requestCause = requestCause;
         }
         updateFlags(flags);
+
+        // Check if REQUEST_CONTENT has been set by calling #requestContent() from a different thread
+        if (hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT) &&
+            isAvailable(RequestLogProperty.REQUEST_CONTENT)) {
+            setNamesIfAbsent();
+        }
     }
 
     private void setNamesIfAbsent() {

diff --git a/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java b/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java
@@ -28,6 +28,9 @@
 import java.util.concurrent.ArrayBlockingQueue;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.BiFunction;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -49,6 +52,7 @@
 import com.linecorp.armeria.common.RpcResponse;
 import com.linecorp.armeria.common.SerializationFormat;
 import com.linecorp.armeria.common.SessionProtocol;
+import com.linecorp.armeria.common.util.ThreadFactories;
 import com.linecorp.armeria.internal.testing.AnticipatedException;
 import com.linecorp.armeria.internal.testing.ImmediateEventLoop;
 import com.linecorp.armeria.server.HttpService;
@@ -544,4 +548,22 @@ void testPendingLogsAlwaysInEventLoop() {
                 .satisfiesAnyOf(t0 -> assertThat(t0).isEqualTo(testThread),
                                 t0 -> assertThat(ctx.eventLoop().inEventLoop(t0)).isTrue()));
     }
+
+    @Test
+    void nameIsAlwaysSet() {
+        final AtomicInteger atomicInteger = new AtomicInteger();
+        final ExecutorService executorService =
+                Executors.newFixedThreadPool(2, ThreadFactories.newThreadFactory("test", true));
+        // a heurestic number of iterations to reproduce #5981
+        final int numIterations = 1000;
+        for (int i = 0; i < numIterations; i++) {
+            final ServiceRequestContext sctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/"));
+            final DefaultRequestLog log = new DefaultRequestLog(sctx);
+            log.defer(RequestLogProperty.REQUEST_CONTENT);
+            executorService.execute(log::endRequest);
+            executorService.execute(() -> log.requestContent(null, null));
+            log.whenRequestComplete().thenRun(atomicInteger::incrementAndGet);
+        }
+        await().untilAsserted(() -> assertThat(atomicInteger).hasValue(numIterations));
+    }
 }