Repository: jhy/jsoup
Pull request #2314 (2025-04-28 02:31:05)

Make Parser instances threadsafe
I've seen examples of people trying to use a Parser in a concurrent manner, and this may succeed in low-concurrency modes, but will fail when under pressure. Adding an explicit lock to ensure safety.


Patch:
diff --git a/src/main/java/org/jsoup/nodes/Document.java b/src/main/java/org/jsoup/nodes/Document.java
index 6be74281..1c8c5db9 100644
--- a/src/main/java/org/jsoup/nodes/Document.java
+++ b/src/main/java/org/jsoup/nodes/Document.java
@@ -37,9 +37,13 @@ public class Document extends Element {
      @see #createShell
      */
     public Document(String namespace, String baseUri) {
+        this(namespace, baseUri, Parser.htmlParser()); // default HTML parser, but overridable
+    }
+
+    private Document(String namespace, String baseUri, Parser parser) {
         super(new Tag("#root", namespace), baseUri);
         this.location = baseUri;
-        this.parser = Parser.htmlParser(); // default, but overridable
+        this.parser = parser;
     }
 
     /**
@@ -293,16 +297,16 @@ public class Document extends Element {
     @Override
     public Document clone() {
         Document clone = (Document) super.clone();
+        if (attributes != null) clone.attributes = attributes.clone();
         clone.outputSettings = this.outputSettings.clone();
-        clone.parser = this.parser.clone();
+        // parser is pointer copy
         return clone;
     }
 
     @Override
     public Document shallowClone() {
-        Document clone = new Document(this.tag().namespace(), baseUri());
-        if (attributes != null)
-            clone.attributes = attributes.clone();
+        Document clone = new Document(this.tag().namespace(), baseUri(), parser); // preserves parser pointer
+        if (attributes != null) clone.attributes = attributes.clone();
         clone.outputSettings = this.outputSettings.clone();
         return clone;
     }
diff --git a/src/main/java/org/jsoup/nodes/Node.java b/src/main/java/org/jsoup/nodes/Node.java
index 406295ac..7a403fbf 100644
--- a/src/main/java/org/jsoup/nodes/Node.java
+++ b/src/main/java/org/jsoup/nodes/Node.java
@@ -867,15 +867,20 @@ public abstract class Node implements Cloneable {
     }
 
     /**
-     * Create a stand-alone, deep copy of this node, and all of its children. The cloned node will have no siblings or
-     * parent node. As a stand-alone object, any changes made to the clone or any of its children will not impact the
-     * original node.
-     * <p>
-     * The cloned node may be adopted into another Document or node structure using {@link Element#appendChild(Node)}.
-     * @return a stand-alone cloned node, including clones of any children
-     * @see #shallowClone()
-     */
-    @SuppressWarnings("MethodDoesntCallSuperMethod") // because it does call super.clone in doClone - analysis just isn't following
+     Create a stand-alone, deep copy of this node, and all of its children. The cloned node will have no siblings.
+     <p><ul>
+     <li>If this node is a {@link LeafNode}, the clone will have no parent.</li>
+     <li>If this node is an {@link Element}, the clone will have a simple owning {@link Document} to retain the
+     configured output settings and parser.</li>
+     </ul></p>
+     <p>The cloned node may be adopted into another Document or node structure using
+     {@link Element#appendChild(Node)}.</p>
+
+     @return a stand-alone cloned node, including clones of any children
+     @see #shallowClone()
+     */
+    @SuppressWarnings("MethodDoesntCallSuperMethod")
+    // because it does call super.clone in doClone - analysis just isn't following
     @Override
     public Node clone() {
         Node thisClone = doClone(null); // splits for orphan
diff --git a/src/main/java/org/jsoup/parser/Parser.java b/src/main/java/org/jsoup/parser/Parser.java
index d853bff8..c16152b1 100644
--- a/src/main/java/org/jsoup/parser/Parser.java
+++ b/src/main/java/org/jsoup/parser/Parser.java
@@ -9,12 +9,15 @@ import org.jspecify.annotations.Nullable;
 import java.io.Reader;
 import java.io.StringReader;
 import java.util.List;
+import java.util.concurrent.locks.ReentrantLock;
 
 /**
  Parses HTML or XML into a {@link org.jsoup.nodes.Document}. Generally, it is simpler to use one of the parse methods in
  {@link org.jsoup.Jsoup}.
- <p>Note that a Parser instance object is not threadsafe. To reuse a Parser configuration in a multi-threaded
- environment, use {@link #newInstance()} to make copies. */
+ <p>Note that a given Parser instance object is threadsafe, but not concurrent. (Concurrent parse calls will
+ synchronize.) To reuse a Parser configuration in a multithreaded environment, use {@link #newInstance()} to make
+ copies.</p>
+ */
 public class Parser implements Cloneable {
     public static final String NamespaceHtml = "http://www.w3.org/1999/xhtml";
     public static final String NamespaceXml = "http://www.w3.org/XML/1998/namespace";
@@ -25,7 +28,8 @@ public class Parser implements Cloneable {
     private ParseErrorList errors;
     private ParseSettings settings;
     private boolean trackPosition = false;
-    @Nullable TagSet tagSet;
+    private @Nullable TagSet tagSet;
+    private final ReentrantLock lock = new ReentrantLock();
 
     /**
      * Create a new Parser, using the specified TreeBuilder
@@ -63,7 +67,12 @@ public class Parser implements Cloneable {
     }
 
     public Document parseInput(Reader inputHtml, String baseUri) {
-        return treeBuilder.parse(inputHtml, baseUri, this);
+        try {
+            lock.lock(); // using a lock vs synchronized to support loom threads
+            return treeBuilder.parse(inputHtml, baseUri, this);
+        } finally {
+            lock.unlock();
+        }
     }
 
     public List<Node> parseFragmentInput(String fragment, @Nullable Element context, String baseUri) {
@@ -71,7 +80,12 @@ public class Parser implements Cloneable {
     }
 
     public List<Node> parseFragmentInput(Reader fragment, @Nullable Element context, String baseUri) {
-        return treeBuilder.parseFragment(fragment, context, baseUri, this);
+        try {
+            lock.lock();
+            return treeBuilder.parseFragment(fragment, context, baseUri, this);
+        } finally {
+            lock.unlock();
+        }
     }
 
     // gets & sets
@@ -87,8 +101,9 @@ public class Parser implements Cloneable {
      * Update the TreeBuilder used when parsing content.
      * @param treeBuilder new TreeBuilder
      * @return this, for chaining
+     * @deprecated unused method, will be removed in 1.21.1
      */
-    public Parser setTreeBuilder(TreeBuilder treeBuilder) {
+    @Deprecated public Parser setTreeBuilder(TreeBuilder treeBuilder) {
         this.treeBuilder = treeBuilder;
         treeBuilder.parser = this;
         return this;


Test patch:
diff --git a/src/test/java/org/jsoup/nodes/DocumentTest.java b/src/test/java/org/jsoup/nodes/DocumentTest.java
index c24640ff..ea9a8a20 100644
--- a/src/test/java/org/jsoup/nodes/DocumentTest.java
+++ b/src/test/java/org/jsoup/nodes/DocumentTest.java
@@ -112,7 +112,7 @@ public class DocumentTest {
         Document clone = doc.clone();
         assertNotSame(doc, clone);
         assertTrue(doc.hasSameValue(clone));
-        assertNotSame(doc.parser(), clone.parser());
+        assertSame(doc.parser(), clone.parser());
         assertNotSame(doc.outputSettings(), clone.outputSettings());
 
         assertEquals("<html><head><title>Hello</title></head><body><p>One</p><p>Two</p></body></html>", TextUtil.stripNewlines(clone.html()));
diff --git a/src/test/java/org/jsoup/nodes/ElementTest.java b/src/test/java/org/jsoup/nodes/ElementTest.java
index 738e8a36..33d0a151 100644
--- a/src/test/java/org/jsoup/nodes/ElementTest.java
+++ b/src/test/java/org/jsoup/nodes/ElementTest.java
@@ -1035,6 +1035,29 @@ public class ElementTest {
         assertEquals(base, d2.baseUri());
     }
 
+    @Test void cloneRetainsParser() {
+        Document htmlDoc = Jsoup.parse("<div><script></script></div>", Parser.htmlParser());
+        Document xmlDoc = Jsoup.parse("<div><script></script></div>", Parser.xmlParser());
+
+        Element hEl = htmlDoc.expectFirst("script");
+        Element hEl2 = hEl.clone();
+        assertNotSame(hEl, hEl2);
+        assertNotSame(hEl.ownerDocument(), hEl2.ownerDocument());
+        assertSame(hEl.ownerDocument().parser(), hEl2.ownerDocument().parser());
+
+        Document doc2 = htmlDoc.clone();
+        assertNotSame(htmlDoc, doc2);
+        assertSame(htmlDoc.parser(), doc2.parser());
+
+        hEl2.append("<foo></foo>"); // we are inside a script, should be parsed as data
+        assertEquals("<foo></foo>", hEl2.data());
+
+        Element xEl = xmlDoc.expectFirst("script");
+        Element xEl2 = xEl.clone();
+        xEl2.append("<foo></foo>"); // in XML, script doesn't mean anything, and so will be parsed as xml
+        assertEquals("<script><foo></foo></script>", xEl2.outerHtml());
+    }
+
     @Test
     public void testTagNameSet() {
         Document doc = Jsoup.parse("<div><i>Hello</i>");
diff --git a/src/test/java/org/jsoup/parser/ParserIT.java b/src/test/java/org/jsoup/parser/ParserIT.java
index 54d757e7..e1904ddc 100644
--- a/src/test/java/org/jsoup/parser/ParserIT.java
+++ b/src/test/java/org/jsoup/parser/ParserIT.java
@@ -1,9 +1,13 @@
 package org.jsoup.parser;
 
 import org.jsoup.nodes.Document;
+import org.jsoup.nodes.Element;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -51,4 +55,69 @@ public class ParserIT {
         assertTrue(System.currentTimeMillis() - start < 20000); // I get ~ 1.5 seconds, but others have reported slower
         // was originally much longer, or stack overflow.
     }
+
+    @Test void parserIsThreadSafe() throws InterruptedException {
+        // tests that a single parser can be called by multiple threads and won't blow up
+        // without the lock, will see many exceptions in parse, and non-equal docs
+        String html = "<div id=1><div id=2><div id=3>Text.</div></div></div>";
+        Parser parser = Parser.htmlParser();
+        Document expectDoc = parser.parseInput(html, "");
+
+        int numThreads = 10;
+        int numLoops = 20;
+        List<Thread> threads = new ArrayList<>(numThreads);
+        List<Document> toCheck = new ArrayList<>(numThreads * numLoops);
+        for (int i = 0; i < numThreads; i++) {
+            Thread thread = new Thread(() -> {
+                for (int j = 0; j < numLoops; j++) {
+                    Document doc = parser.parseInput(html, "");
+                    toCheck.add(doc);
+                }
+            });
+            threads.add(thread);
+            thread.start();
+        }
+
+        for (Thread thread : threads) {
+            thread.join();
+        }
+
+        for (Document doc : toCheck) {
+            assertTrue(doc.hasSameValue(expectDoc));
+        }
+    }
+
+    @Test void parserIsThreadSafeWithCloneAndAppend() throws InterruptedException {
+        // tests that a single parser can be called by multiple threads via Element.clone().append()
+        String html = "<div id=1><div id=2><div id=3></div></div></div>";
+        String append = "<div id=4>Text.</div>";
+        Parser parser = Parser.htmlParser();
+        Document baseDoc = parser.parseInput(html, "");
+        Element baseElement = baseDoc.expectFirst("#3");
+
+        int numThreads = 10;
+        int numLoops = 20;
+        List<Thread> threads = new ArrayList<>(numThreads);
+        List<Element> toCheck = new ArrayList<>(numThreads * numLoops);
+        for (int i = 0; i < numThreads; i++) {
+            Thread thread = new Thread(() -> {
+                for (int j = 0; j < numLoops; j++) {
+                    Element cloned = baseElement.clone();
+                    cloned.append(append); // invokes the parser internally - parseFragment
+                    toCheck.add(cloned);
+                }
+            });
+            threads.add(thread);
+            thread.start();
+        }
+
+        for (Thread thread : threads) {
+            thread.join();
+        }
+
+        baseElement.append(append);
+        for (Element element : toCheck) {
+            assertTrue(element.hasSameValue(baseElement));
+        }
+    }
 }