search

pdvrieze / xmlutil

XML Serialization library for Kotlin
https://pdvrieze.github.io/xmlutil/
Apache License 2.0
363 stars 30 forks source link

ServiceLoader should not be a global singleton #211

Closed felixkrull-neuland closed 1 month ago

felixkrull-neuland commented 1 month ago

In https://github.com/pdvrieze/xmlutil/blob/master/core/base/src/jvmMain/kotlin/nl/adaptivity/xmlutil/XmlStreaming.jvm.kt#L57, a ServiceLoader is created as a global singleton. However, ServiceLoader is not thread-safe according to the docs so this incorrectly uses a ServiceLoader across multiple threads.

In particular, I'm getting a NoSuchElementException when using XML.encodeToString in possibly-concurrent tests, with a few different stacktraces:

java.util.NoSuchElementException
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.nextService(ServiceLoader.java:1255)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.next(ServiceLoader.java:1286)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.next(ServiceLoader.java:1116)
        at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1316)
        at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1306)
        at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
        at kotlin.collections.CollectionsKt___CollectionsKt.firstOrNull(_Collections.kt:279)
        at nl.adaptivity.xmlutil.XmlStreaming.getFactory(XmlStreaming.jvm.kt:71)
        at nl.adaptivity.xmlutil.XmlStreaming.newWriter(XmlStreaming.jvm.kt:118)
        at nl.adaptivity.xmlutil.XmlStreaming_jvmKt.newWriter(XmlStreaming.jvm.kt:331)
        at nl.adaptivity.xmlutil.serialization.XML.encodeToString(XML.kt:109)
        at nl.adaptivity.xmlutil.serialization.XML.encodeToString(XML.kt:97)
        <my own code starts here...>
java.util.NoSuchElementException
        at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1318)
        at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1306)
        at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
        at kotlin.collections.CollectionsKt___CollectionsKt.firstOrNull(_Collections.kt:279)
        <rest as above>
java.util.NoSuchElementException
        at java.base/java.lang.CompoundEnumeration.nextElement(ClassLoader.java:2770)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1213)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1228)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273)
        at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309)
        at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393)
        at kotlin.collections.CollectionsKt___CollectionsKt.firstOrNull(_Collections.kt:277)
        <rest as above>

The general flakiness of the error and stacktraces makes sense for a concurrency problem to me.

xmlutil: serialization-jvm 0.90.0-RC1 Java: OpenJDK 21.0.3

felixkrull-neuland commented 1 month ago

Edit: never mind, I was wrong. CompoundEnumeration increments the index in hasMoreElements, but doesn't actually advance the underlying enumeration so its state handling is probably correct.

(This is not directly relevant to the issue so I'm adding it as a comment.)

What I find kinda weird about this: there's a JDK bug that reports the third of my stacktraces when using a ServiceLoader across many threads: https://bugs.openjdk.org/browse/JDK-8230843 That bug was closed (which pointed me to ServiceLoader not being thread-safe). But looking at CompoundEnumeration, the class from the stacktrace, it seems... wrong? I'm pretty sure hasMoreElements shouldn't change the iteration state.

pdvrieze commented 1 month ago

This code was there quite some time. I've changed it to use a getter instead of lazy. And I made the factory variable volatile. although this probably isn't needed (factory clobbering is not an issue in this case). But given that serialization goes through the factory pseudo-lazy variable the loader is only used initially, you can set the factory by hand, or initialise it on a single thread. Which version are you using? The latest version should have partially addressed this. I've just pushed a fix for the concurrency issue (it should be available soon-ish as snapshot).

pdvrieze commented 1 month ago

This should be resolved in RC2

felixkrull-neuland commented 1 month ago

yeah, it seems to have fixed it, thanks!