PrettyPrinter is not threadsafe.

[EdgeCaseBerg]

The PrettyPrinter, according to the docs:

Class for pretty printing. After instantiating, you can use the format() and formatNode() methods to convert XML to a formatted string. The class can be reused to pretty print any number of XML nodes.

The last sentence about being able to print any number of xml nodes I took to mean that the class operates in a pure manner and I could use this a concurrent environment. That was probably a bad assumption on my part. But here's a test case to reproduce the problem:

import scala.concurrent._
import scala.xml._
import java.util.concurrent.CountDownLatch
import scala.concurrent.ExecutionContext.Implicits.global

val p = new PrettyPrinter(width = Int.MaxValue, step = 2)
def xmlNode() = <A><B><C>Hello</C><D>There</D></B></A>
val latch = new CountDownLatch(1)
 
val futurePretty = (0 to 10).map { _ =>
	Future {
		val x = xmlNode()
		latch.await()
		p.format(x)
	}
}

latch.countDown()

val l = Future.sequence(futurePretty)

l.value.map(println)

On a run on my machine I see the result of that last console command as:

scala> l.value.map(println)
Success(Vector(<A>
  <B>
  <B>
    <C>Hello</C>
    <C>Hello</C>
    <C>Hello</C>
    <D>There</D>
  </B>
    <D>There</D>
  </B></B>
    <D>There</D>
</A>, <A>
  <B>
  <B>
    <C>Hello</C>
    <C>Hello</C>
... snipped for brevity...

Which clearly indicates to me that the pretty printer is using some buffer internal to the instance and therefore calling .format from different threads causes this buffer to be filled with values from each calling thread. This jumbles things up a bit.

If it's unlikely that this issue will be fixed at any point, could we please update the documentation to leave an explicit note that the class is not thread safe and should not be used like that?

[ashawley]

The last sentence about being able to print any number of xml nodes I took to mean that the class operates in a pure manner and I could use this a concurrent environment.

Yeah, I guess it was intended to convey that you can re-use the instance, but not that it can be used simultaneously. I agree the wording is ambiguous.

You have proven it's not thread-safe. I rewrote your example using the parallel collections library and added some Ints to keep track of things and can confirm there's a problem.

scala> def xmlNode(x: Int) = 
     |   <A><B><C>Hello {x.toString}</C><D>World {x.toString}</D></B></A>

scala> val pp = new scala.xml.PrettyPrinter(80, 2)

scala> (0 to 9).par.map(x => pp.format(xmlNode(x)))
res1: scala.collection.parallel.immutable.ParSeq[String] =
ParVector("  <B>
    <C>Hello 6</C>
    <D>World 6</D>
  </B>
</A><A>
    <D>World 0</D>
  </B>
</A>", "    <C>Hello 4</C>
    <D>World 4</D>
  </B>
</A><A>
<...

That it's not thread-safe is unsurprising since you need to instantiate with new just to use a function, and because there are subsequently a lot of vars in the implementation of format.

I'd be amenable to just deleting the ambiguous line, and then replacing it with "This class is not thread safe."

[NthPortal]

I don't think the line should necessarily be deleted; just clarified to say that it's not thread-safe. It is conceivable that a PrettyPrinter could be single use (or at least require resetting in between uses, which this doesn't), and I think it's valuable to state that that is not the case.

[EdgeCaseBerg]

Right, something like:

def someMethodCalledByManyThreads(things: Seq[Node]) = {
    val pp = new scala.xml.PrettyPrinter(80, 2)
    things.map(pp.format)
}

Where you can use the pretty printer multiple times within it's owning thread is not a problem and good to know you can do. But definitely a 👍 for adding in a note about it not being threadsafe. It's pretty trivial to add in an @note This class is not thread safe to the docstring of the class. So I've opened up a PR for that: #215