Use a ReentrantLock in HttpConnection

Simplifies how we grab the lock.

Also tweaked the IT test so it completes as soon as all exceptions thrown, vs waiting for the SlowRider timeout.

Author: jhy

src/main/java/org/jsoup/helper/HttpConnection.java

@@ -40,6 +40,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.regex.Pattern;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.Inflater;
@@ -621,7 +622,7 @@ public static class Request extends HttpConnection.Base<Connection.Request> impl
         @Nullable RequestAuthenticator authenticator;
         private @Nullable Progress<Connection.Response> responseProgress;
 
-        private volatile boolean executing = false;
+        private final ReentrantLock executing = new ReentrantLock(); // detects and warns if same request used concurrently
 
         Request() {
             super();
@@ -654,7 +655,6 @@ public static class Request extends HttpConnection.Base<Connection.Request> impl
             cookieManager = copy.cookieManager;
             authenticator = copy.authenticator;
             responseProgress = copy.responseProgress;
-            executing = false;
         }
 
         @Override @Nullable
@@ -847,10 +847,7 @@ static Response execute(HttpConnection.Request req) throws IOException {
         }
 
         static Response execute(HttpConnection.Request req, @Nullable Response prevRes) throws IOException {
-            synchronized (req) {
-                Validate.isFalse(req.executing, "Multiple threads were detected trying to execute the same request concurrently. Make sure to use Connection#newRequest() and do not share an executing request between threads.");
-                req.executing = true;
-            }
+            Validate.isTrue(req.executing.tryLock(), "Multiple threads were detected trying to execute the same request concurrently. Make sure to use Connection#newRequest() and do not share an executing request between threads.");
             Validate.notNullParam(req, "req");
             URL url = req.url();
             Validate.notNull(url, "URL must be specified to connect");
@@ -890,7 +887,6 @@ else if (supportsBody)
                     URL redir = StringUtil.resolve(req.url(), location);
                     req.url(redir);
 
-                    req.executing = false;
                     return execute(req, res);
                 }
                 if ((res.statusCode < 200 || res.statusCode >= 400) && !req.ignoreHttpErrors())
@@ -932,7 +928,7 @@ else if (res.hasHeaderWithValue(CONTENT_ENCODING, "deflate"))
                 if (res != null) res.safeClose(); // will be non-null if got to conn
                 throw e;
             } finally {
-                req.executing = false;
+                req.executing.unlock();
 
                 // detach any thread local auth delegate
                 if (req.authenticator != null)

src/test/java/org/jsoup/integration/SessionIT.java

@@ -75,15 +75,28 @@ public void multiThread() throws InterruptedException {
     // test that we throw a nice clear exception if you try to multi-thread by forget .newRequest()
     @Test
     public void multiThreadWithoutNewRequestBlowsUp() throws InterruptedException {
-        int numThreads = 20;
-        String url = SlowRider.Url + "?" + SlowRider.MaxTimeParam + "=10000"; // this makes sure that the first req is still executing whilst the others run
+        int numThreads = 5;
+        String url = SlowRider.Url + "?" + SlowRider.MaxTimeParam + "=20000"; // this makes sure that the first req is still executing whilst the others run
         String title = "Slow Rider";
 
         ThreadCatcher catcher = new ThreadCatcher();
         Connection session = Jsoup.newSession();
 
-        Thread[] threads = new Thread[numThreads];
+        // run first slow request
         AtomicInteger successful = new AtomicInteger();
+        Thread slow = new Thread(() -> {
+            try {
+                Document doc = session.url(url).get();
+                assertEquals(title, doc.title());
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);
+            }
+        });
+        slow.start();
+        Thread.sleep(100); // yield so that thread can start before the next do
+
+        // spawn others, should fail
+        Thread[] threads = new Thread[numThreads];
         for (int threadNum = 0; threadNum < numThreads; threadNum++) {
             Thread thread = new Thread(() -> {
                 try {
@@ -104,6 +117,8 @@ public void multiThreadWithoutNewRequestBlowsUp() throws InterruptedException {
         for (Thread thread : threads) {
             thread.join();
         }
+        // cancel the slow runner so we can wrap up the test quicker
+        slow.interrupt();
 
         // only one should have passed, rest should have blown up (assuming the started whilst other was running)
         //assertEquals(numThreads - 1, catcher.multiThreadExceptions.get());
@@ -114,7 +129,7 @@ public void multiThreadWithoutNewRequestBlowsUp() throws InterruptedException {
          macOS runners, which appear overtaxed. That makes this test flaky. So we relax the test conditions, and make
          sure at least just one passed and one failed. That's OK in prod as well, because we are only concerned about
          concurrent execution, which the impl does detect correctly. */
-        assertTrue(successful.get() > 0);
+        assertEquals(0, successful.get());
         assertTrue(catcher.multiThreadExceptions.get() > 0);
         assertEquals(catcher.multiThreadExceptions.get(), catcher.exceptionCount.get()); // all exceptions are multi-threaded
     }