Xmlutils thread safety

stem-plugin/src/main/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtils.kt

@@ -14,24 +14,21 @@ import org.w3c.dom.Node
 import org.xml.sax.InputSource
 
 object XmlUtils {
-    private val docBuilder by lazy {
-        DocumentBuilderFactory
-            .newInstance()
-            .newDocumentBuilder()
-    }
-    private val contentExtractor by lazy {
-        val transformer = TransformerFactory.newInstance().newTransformer()
-        transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")
-        transformer
+    private fun createContentExtractor() = TransformerFactory.newInstance().newTransformer().apply {
+        setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")
     }
+
     private val OUTER_XML_TAGS_PATTERN = Regex("^<[^>]*>|<[^>]*>\$")
 
     fun stringResourceModelToElement(
         stringResourceModel: StringResource,
         namespaceNameProvider: NamespaceNameProvider
     ): Element {
         val reader = StringReader("<string>${stringResourceModel.text}</string>")
-        val strElement = docBuilder.parse(InputSource(reader)).documentElement
+        val strElement = DocumentBuilderFactory.newInstance()
+            .newDocumentBuilder()
+            .parse(InputSource(reader))
+            .documentElement
         for (it in stringResourceModel.attributes) {
             it.namespace?.let { namespace ->
                 val namespaceName = namespaceNameProvider.getNameFor(namespace)
@@ -45,7 +42,7 @@ object XmlUtils {
         val outText = StringWriter()
         val streamResult = StreamResult(outText)
         return try {
-            contentExtractor.transform(DOMSource(node), streamResult)
+            createContentExtractor().transform(DOMSource(node), streamResult)
             val text = outText.toString()
             return OUTER_XML_TAGS_PATTERN.replace(text, "")
         } catch (e: TransformerException) {
@@ -56,4 +53,4 @@ object XmlUtils {
     interface NamespaceNameProvider {
         fun getNameFor(namespaceValue: String): String
     }
-}
+}

stem-plugin/src/test/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtilsConcurrencyTest.kt

@@ -0,0 +1,113 @@
+package com.likethesalad.stem.modules.common.helpers.resources.utils
+
+import com.likethesalad.android.protos.Attribute
+import com.likethesalad.android.protos.StringResource
+import com.likethesalad.stem.testutils.named
+import java.util.concurrent.CyclicBarrier
+import java.util.concurrent.Executors
+import java.util.concurrent.TimeUnit
+import java.util.concurrent.atomic.AtomicInteger
+import java.util.concurrent.atomic.AtomicReference
+import org.junit.jupiter.api.Test
+import org.w3c.dom.Element
+
+/**
+ * Reproduces https://github.com/LikeTheSalad/android-stem/issues/332.
+ *
+ * [XmlUtils] previously held one shared DocumentBuilder and one shared Transformer, neither of
+ * which is thread safe. Gradle runs the tasks that reach this code directly on its own task
+ * execution threads, so parallel variant tasks raced on those instances.
+ */
+class XmlUtilsConcurrencyTest {
+
+    private val noOpNsProvider = object : XmlUtils.NamespaceNameProvider {
+        override fun getNameFor(namespaceValue: String): String {
+            throw UnsupportedOperationException()
+        }
+    }
+
+    /**
+     * Verifies that concurrent calls do not share a DocumentBuilder. Before the regression was
+     * fixed, this failed with SAXException "FWK005 parse may not be called while parsing".
+     */
+    @Test
+    fun checkStringResourceModelToElementUnderConcurrentUse() {
+        runConcurrently { threadIndex, iteration ->
+            val name = "string_${threadIndex}_$iteration"
+            val text = "content $threadIndex-$iteration"
+
+            val element = XmlUtils.stringResourceModelToElement(
+                StringResource.named(name, text, listOf(Attribute("extra", "extra_$threadIndex", null))),
+                noOpNsProvider
+            )
+
+            check(element.textContent == text) {
+                "Expected text <$text> but was <${element.textContent}>"
+            }
+            check(element.attributes.getNamedItem("name").textContent == name) {
+                "Expected name <$name> but was <${element.attributes.getNamedItem("name").textContent}>"
+            }
+        }
+    }
+
+    /**
+     * Verifies that concurrent calls do not share a Transformer. The inline `<b>` tag makes
+     * corrupted output observable.
+     */
+    @Test
+    fun checkGetContentsUnderConcurrentUse() {
+        // Given: one element per thread, built up front so that only getContents runs concurrently.
+        val expectedTexts = (0 until THREAD_COUNT).map { "content $it <b>bold $it</b>" }
+        val elements: List<Element> = expectedTexts.mapIndexed { index, text ->
+            XmlUtils.stringResourceModelToElement(
+                StringResource.named("string_$index", text, emptyList()),
+                noOpNsProvider
+            )
+        }
+
+        runConcurrently { threadIndex, _ ->
+            val expected = expectedTexts[threadIndex]
+            val contents = XmlUtils.getContents(elements[threadIndex])
+
+            check(contents == expected) {
+                "Expected contents <$expected> but was <$contents>"
+            }
+        }
+    }
+
+    private fun runConcurrently(body: (threadIndex: Int, iteration: Int) -> Unit) {
+        val executor = Executors.newFixedThreadPool(THREAD_COUNT)
+        val barrier = CyclicBarrier(THREAD_COUNT)
+        val failureCount = AtomicInteger()
+        val firstFailure = AtomicReference<Throwable>()
+
+        try {
+            val futures = (0 until THREAD_COUNT).map { threadIndex ->
+                executor.submit {
+                    for (iteration in 0 until ITERATIONS) {
+                        barrier.await()
+                        try {
+                            body(threadIndex, iteration)
+                        } catch (e: Throwable) {
+                            failureCount.incrementAndGet()
+                            firstFailure.compareAndSet(null, e)
+                        }
+                    }
+                }
+            }
+            futures.forEach { it.get(TIMEOUT_SECONDS, TimeUnit.SECONDS) }
+        } finally {
+            executor.shutdownNow()
+        }
+
+        firstFailure.get()?.let { cause ->
+            throw AssertionError("${failureCount.get()} concurrent operations failed", cause)
+        }
+    }
+
+    companion object {
+        private const val THREAD_COUNT = 8
+        private const val ITERATIONS = 500
+        private const val TIMEOUT_SECONDS = 60L
+    }
+}