8308310: HttpClient: Avoid logging or locking from within synchronized blocks

Reviewed-by: jpai

Author: dfuch

src/java.net.http/share/classes/jdk/internal/net/http/BufferingSubscriber.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -57,6 +57,7 @@ public class BufferingSubscriber<T> implements TrustedSubscriber<T>
     private volatile DownstreamSubscription downstreamSubscription;
 
     /** Must be held when accessing the internal buffers. */
+    // using a monitor here is fine: no logging while holding it
     private final Object buffersLock = new Object();
     /** The internal buffers holding the buffered data. */
     private ArrayList<ByteBuffer> internalBuffers;

src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -40,6 +40,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.Flow;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
 import jdk.internal.net.http.common.FlowTube;
 import jdk.internal.net.http.common.Logger;
@@ -57,11 +58,12 @@ final class ConnectionPool {
 
     // Pools of idle connections
 
+    private final ReentrantLock stateLock = new ReentrantLock();
     private final HashMap<CacheKey,LinkedList<HttpConnection>> plainPool;
     private final HashMap<CacheKey,LinkedList<HttpConnection>> sslPool;
     private final ExpiryList expiryList;
     private final String dbgTag; // used for debug
-    boolean stopped;
+    volatile boolean stopped;
 
     /**
      * Entries in connection pool are keyed by destination address and/or
@@ -139,7 +141,7 @@ final String dbgString() {
         return dbgTag;
     }
 
-    synchronized void start() {
+    void start() {
         assert !stopped : "Already stopped";
     }
 
@@ -149,9 +151,21 @@ static CacheKey cacheKey(boolean secure, InetSocketAddress destination,
         return new CacheKey(secure, destination, proxy);
     }
 
-    synchronized HttpConnection getConnection(boolean secure,
-                                              InetSocketAddress addr,
-                                              InetSocketAddress proxy) {
+    HttpConnection getConnection(boolean secure,
+                                 InetSocketAddress addr,
+                                 InetSocketAddress proxy) {
+        if (stopped) return null;
+        stateLock.lock();
+        try {
+            return getConnection0(secure, addr, proxy);
+        } finally {
+            stateLock.unlock();
+        }
+    }
+
+    private HttpConnection getConnection0(boolean secure,
+                                          InetSocketAddress addr,
+                                          InetSocketAddress proxy) {
         if (stopped) return null;
         // for plain (unsecure) proxy connection the destination address is irrelevant.
         addr = secure || proxy == null ? addr : null;
@@ -185,7 +199,8 @@ void returnToPool(HttpConnection conn, Instant now, long keepAlive) {
 
         // it's possible that cleanup may have been called.
         HttpConnection toClose = null;
-        synchronized(this) {
+        stateLock.lock();
+        try {
             if (cleanup.isDone()) {
                 return;
             } else if (stopped) {
@@ -203,6 +218,8 @@ void returnToPool(HttpConnection conn, Instant now, long keepAlive) {
                 putConnection(conn, sslPool);
             }
             expiryList.add(conn, now, keepAlive);
+        } finally {
+            stateLock.unlock();
         }
         if (toClose != null) {
             if (debug.on()) {
@@ -243,7 +260,7 @@ private CleanupTrigger registerCleanupTrigger(HttpConnection conn) {
     removeFromPool(HttpConnection c,
                    HashMap<CacheKey,LinkedList<HttpConnection>> pool) {
         //System.out.println("cacheCleaner removing: " + c);
-        assert Thread.holdsLock(this);
+        assert stateLock.isHeldByCurrentThread();
         CacheKey k = c.cacheKey();
         List<HttpConnection> l = pool.get(k);
         if (l == null || l.isEmpty()) {
@@ -288,7 +305,8 @@ long purgeExpiredConnectionsAndReturnNextDeadline(Instant now) {
         if (!expiryList.purgeMaybeRequired()) return nextPurge;
 
         List<HttpConnection> closelist;
-        synchronized (this) {
+        stateLock.lock();
+        try {
             closelist = expiryList.purgeUntil(now);
             for (HttpConnection c : closelist) {
                 if (c instanceof PlainHttpConnection) {
@@ -302,6 +320,8 @@ long purgeExpiredConnectionsAndReturnNextDeadline(Instant now) {
             nextPurge = now.until(
                     expiryList.nextExpiryDeadline().orElse(now),
                     ChronoUnit.MILLIS);
+        } finally {
+            stateLock.unlock();
         }
         closelist.forEach(this::close);
         return nextPurge;
@@ -316,14 +336,17 @@ private void close(HttpConnection c) {
     void stop() {
         List<HttpConnection> closelist = Collections.emptyList();
         try {
-            synchronized (this) {
+            stateLock.lock();
+            try {
                 stopped = true;
                 closelist = expiryList.stream()
                     .map(e -> e.connection)
                     .collect(Collectors.toList());
                 expiryList.clear();
                 plainPool.clear();
                 sslPool.clear();
+            } finally {
+                stateLock.unlock();
             }
         } finally {
             closelist.forEach(this::close);
@@ -354,28 +377,25 @@ private static final class ExpiryList {
 
         // A loosely accurate boolean whose value is computed
         // at the end of each operation performed on ExpiryList;
-        // Does not require synchronizing on the ConnectionPool.
+        // Does not require holding the ConnectionPool stateLock.
         boolean purgeMaybeRequired() {
             return mayContainEntries;
         }
 
         // Returns the next expiry deadline
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         Optional<Instant> nextExpiryDeadline() {
             if (list.isEmpty()) return Optional.empty();
             else return Optional.of(list.getLast().expiry);
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         HttpConnection removeOldest() {
             ExpiryEntry entry = list.pollLast();
             return entry == null ? null : entry.connection;
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         void add(HttpConnection conn) {
             add(conn, Instant.now(), KEEP_ALIVE_TIMEOUT);
         }
@@ -408,8 +428,7 @@ void add(HttpConnection conn, Instant now, long keepAlive) {
             mayContainEntries = true;
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         void remove(HttpConnection c) {
             if (c == null || list.isEmpty()) return;
             ListIterator<ExpiryEntry> li = list.listIterator();
@@ -423,8 +442,7 @@ void remove(HttpConnection c) {
             }
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool.
+        // should only be called while holding the ConnectionPool stateLock.
         // Purge all elements whose deadline is before now (now included).
         List<HttpConnection> purgeUntil(Instant now) {
             if (list.isEmpty()) return Collections.emptyList();
@@ -450,25 +468,22 @@ List<HttpConnection> purgeUntil(Instant now) {
             return closelist;
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         java.util.stream.Stream<ExpiryEntry> stream() {
             return list.stream();
         }
 
-        // should only be called while holding a synchronization
-        // lock on the ConnectionPool
+        // should only be called while holding the ConnectionPool stateLock.
         void clear() {
             list.clear();
             mayContainEntries = false;
         }
     }
 
     // Remove a connection from the pool.
-    // should only be called while holding a synchronization
-    // lock on the ConnectionPool
+    // should only be called while holding the ConnectionPool stateLock.
     private void removeFromPool(HttpConnection c) {
-        assert Thread.holdsLock(this);
+        assert stateLock.isHeldByCurrentThread();
         if (c instanceof PlainHttpConnection) {
             removeFromPool(c, plainPool);
         } else {
@@ -478,7 +493,16 @@ private void removeFromPool(HttpConnection c) {
     }
 
     // Used by tests
-    synchronized boolean contains(HttpConnection c) {
+    boolean contains(HttpConnection c) {
+        stateLock.lock();
+        try {
+            return contains0(c);
+        } finally {
+            stateLock.unlock();
+        }
+    }
+
+    private boolean contains0(HttpConnection c) {
         final CacheKey key = c.cacheKey();
         List<HttpConnection> list;
         if ((list = plainPool.get(key)) != null) {
@@ -494,9 +518,12 @@ void cleanup(HttpConnection c, Throwable error) {
         if (debug.on())
             debug.log("%s : ConnectionPool.cleanup(%s)",
                     String.valueOf(c.getConnectionFlow()), error);
-        synchronized(this) {
+        stateLock.lock();
+        try {
             removeFromPool(c);
             expiryList.remove(c);
+        } finally {
+            stateLock.unlock();
         }
         c.close();
     }

src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java

@@ -288,7 +288,7 @@ private void checkCancelled() {
         IOException cause = null;
         CompletableFuture<? extends ExchangeImpl<T>> cf = null;
         if (failed != null) {
-            synchronized(this) {
+            synchronized (this) {
                 cause = failed;
                 impl = exchImpl;
                 cf = exchangeCF;
@@ -371,7 +371,7 @@ synchronized IOException getCancelCause() {
         // instead - as we need CAS semantics.
         synchronized (this) { exchangeCF = cf; };
         res = cf.whenComplete((r,x) -> {
-            synchronized(Exchange.this) {
+            synchronized (Exchange.this) {
                 if (exchangeCF == cf) exchangeCF = null;
             }
         });

src/java.net.http/share/classes/jdk/internal/net/http/Http1AsyncReceiver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -419,7 +419,7 @@ void clear() {
     }
 
     void subscribe(Http1AsyncDelegate delegate) {
-        synchronized(this) {
+        synchronized (this) {
             pendingDelegateRef.set(delegate);
         }
         if (queue.isEmpty()) {
@@ -443,12 +443,16 @@ long remaining() {
     }
 
     void unsubscribe(Http1AsyncDelegate delegate) {
-        synchronized(this) {
+        boolean unsubscribed = false;
+        synchronized (this) {
             if (this.delegate == delegate) {
-                if (debug.on()) debug.log("Unsubscribed %s", delegate);
                 this.delegate = null;
+                unsubscribed = true;
             }
         }
+        if (unsubscribed) {
+            if (debug.on()) debug.log("Unsubscribed %s", delegate);
+        }
     }
 
     // Callback: Consumer of ByteBuffer