diff --git a/stem-plugin/src/main/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtils.kt b/stem-plugin/src/main/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtils.kt index 15b3635b..34d6d91a 100644 --- a/stem-plugin/src/main/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtils.kt +++ b/stem-plugin/src/main/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtils.kt @@ -14,16 +14,10 @@ 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( @@ -31,7 +25,10 @@ object XmlUtils { namespaceNameProvider: NamespaceNameProvider ): Element { val reader = StringReader("${stringResourceModel.text}") - 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 } -} \ No newline at end of file +} diff --git a/stem-plugin/src/test/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtilsConcurrencyTest.kt b/stem-plugin/src/test/java/com/likethesalad/stem/modules/common/helpers/resources/utils/XmlUtilsConcurrencyTest.kt new file mode 100644 index 00000000..2dd97672 --- /dev/null +++ b/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 `` 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 bold $it" } + val elements: List = 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() + + 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 + } +}