(httpcomponents-core) branch 5.4.x updated: ComplexCancellable: Fix race condition

[rs...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rschmitt pushed a commit to branch 5.4.x
in repository https://gitbox.apache.org/repos/asf/httpcomponents-core.git


The following commit(s) were added to refs/heads/5.4.x by this push:
     new 15e9de48f ComplexCancellable: Fix race condition
15e9de48f is described below

commit 15e9de48fa13f269879cadb729b5c017d47e9c93
Author: Ryan Schmitt <rs...@apache.org>
AuthorDate: Wed Jan 14 21:08:56 2026 -0800

    ComplexCancellable: Fix race condition
    
    I discovered a race condition while investigating a sporadic test
    failure in the client:
    
    ```
    [ERROR]   HttpMinimalIntegrationTests$H2Tls>AbstractHttpAsyncFundamentalsTest.testSequentialGetRequests:86 » Execution org.apache.hc.core5.http2.H2StreamResetException: Cancelled
    ```
    
    I discovered that two threads are somehow racing when calling
    `ComplexCancellable.setDependency()`:
    
    ```
    setDependency dep=org.apache.hc.client5.http.impl.async.MinimalH2AsyncClient$$Lambda$655/251536271@633478cb ct=ForkJoinPool-1-worker-2
    setDependency dep=[id=1, reserved=false, removeClosed=false, localClosed=false, localReset=false] ct=httpclient-dispatch-1
    setDependency dep=org.apache.hc.client5.http.impl.async.MinimalH2AsyncClient$$Lambda$655/251536271@2e66671a ct=ForkJoinPool-1-worker-2
    setDependency dep=[id=3, reserved=false, removeClosed=false, localClosed=false, localReset=false] ct=httpclient-dispatch-1
    setDependency dep=org.apache.hc.client5.http.impl.async.MinimalH2AsyncClient$$Lambda$655/251536271@32f4e96 ct=ForkJoinPool-1-worker-2
    setDependency dep=[id=5, reserved=false, removeClosed=false, localClosed=false, localReset=false] ct=httpclient-dispatch-1
    ```
    
    The implementation of `setDependency` was performing a single CAS and
    interpreting failure to mean that the mark bit had been set -- in other
    words, that another thread had called `cancel()`, rather than
    `setDependency()`.
    
    This change corrects the implementation of `setDependency` by having it
    run the CAS in a loop. On each update failure, it checks if the mark bit
    has been set (nothing ever unsets it). If so, it calls
    `dependency.cancel()` and returns; else, it just tries again.
---
 .../hc/core5/concurrent/ComplexCancellable.java    |  8 ++++---
 .../core5/concurrent/TestComplexCancellable.java   | 27 ++++++++++++++++++++--
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/httpcore5/src/main/java/org/apache/hc/core5/concurrent/ComplexCancellable.java b/httpcore5/src/main/java/org/apache/hc/core5/concurrent/ComplexCancellable.java
index 1b4321975..eabfc530a 100644
--- a/httpcore5/src/main/java/org/apache/hc/core5/concurrent/ComplexCancellable.java
+++ b/httpcore5/src/main/java/org/apache/hc/core5/concurrent/ComplexCancellable.java
@@ -53,9 +53,11 @@ public boolean isCancelled() {
     @Override
     public void setDependency(final Cancellable dependency) {
         Args.notNull(dependency, "dependency");
-        final Cancellable actualDependency = dependencyRef.getReference();
-        if (!dependencyRef.compareAndSet(actualDependency, dependency, false, false)) {
-            dependency.cancel();
+        while (!dependencyRef.compareAndSet(dependencyRef.getReference(), dependency, false, false)) {
+            if (dependencyRef.isMarked()) {
+                dependency.cancel();
+                return;
+            }
         }
     }
 
diff --git a/httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestComplexCancellable.java b/httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestComplexCancellable.java
index 42806b9b8..e728bc645 100644
--- a/httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestComplexCancellable.java
+++ b/httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestComplexCancellable.java
@@ -27,11 +27,13 @@
 package org.apache.hc.core5.concurrent;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 
 import org.hamcrest.CoreMatchers;
-import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
+import java.util.concurrent.CountDownLatch;
+
 class TestComplexCancellable {
 
     @Test
@@ -41,7 +43,7 @@ void testCancelled() {
         final BasicFuture<Object> dependency1 = new BasicFuture<>(null);
         cancellable.setDependency(dependency1);
 
-        Assertions.assertFalse(cancellable.isCancelled());
+        assertFalse(cancellable.isCancelled());
 
         cancellable.cancel();
         assertThat(cancellable.isCancelled(), CoreMatchers.is(true));
@@ -52,4 +54,25 @@ void testCancelled() {
         assertThat(dependency2.isCancelled(), CoreMatchers.is(true));
     }
 
+    @Test
+    void testSetDependencyRace() throws InterruptedException {
+        final ComplexCancellable cancellable = new ComplexCancellable();
+        final BasicFuture<Object> dependency1 = new BasicFuture<>(null);
+        final BasicFuture<Object> dependency2 = new BasicFuture<>(null);
+        final CountDownLatch latch = new CountDownLatch(2);
+
+        for (int i = 0; i < 2; i++) {
+            new Thread(() -> {
+                for (int j = 0; j < 5_000; j++) {
+                    cancellable.setDependency(dependency1);
+                    cancellable.setDependency(dependency2);
+                }
+                latch.countDown();
+            }).start();
+        }
+        latch.await();
+
+        assertFalse(dependency1.isCancelled());
+        assertFalse(dependency2.isCancelled());
+    }
 }