pjfanning commented on PR #1241: URL: https://github.com/apache/poi/pull/1241#issuecomment-5529618086
Audited the other XML factories in `XMLHelper` after a question about whether any of them are actually thread-safe. Summary of what I found, since it affects how this PR treats them differently. **What is shared.** Only three factories are cached and shared: `documentBuilderFactory` and `saxFactory` (both long-standing) and the `transformerFactory` added here. `newXMLInputFactory()`, `newXMLOutputFactory()`, `newXMLEventFactory()` and `getSchemaFactory()` all build a fresh instance per call, so they never share state. **What the spec says.** `TransformerFactory`'s javadoc states "Different `TransformerFactories` can be used concurrently by different `Thread`s", which implies a single instance is not meant to be shared. The `DocumentBuilderFactory` and `SAXParserFactory` javadocs say nothing about thread safety in either direction — so the old "newDocumentBuilder is thread-safe" comment claimed more than JAXP actually gives. **What the implementations do.** `DocumentBuilderFactoryImpl.newDocumentBuilder()` and `SAXParserFactoryImpl.newSAXParser()` only read the factory's `features`/`attributes` maps and hand them to the new parser; neither mutates factory state during creation. POI configures these once in the static initializer and never mutates them afterwards, so the shared use is safe with the JDK implementations. **Stress test**, 16 threads x 3000 iterations each, creating a parser from one shared factory and then actually parsing/transforming with it — no failures in 48,000 operations for any of them, including `TransformerFactory.newTransformer()` with no lock: ``` DocumentBuilderFactory.newDocumentBuilder() OK (48000/48000 ops) SAXParserFactory.newSAXParser() OK (48000/48000 ops) TransformerFactory.newTransformer() UNSYNCED OK (48000/48000 ops) TransformerFactory.newTransformer() SYNCED OK (48000/48000 ops) ``` **Why not lock all three for consistency.** Because it measurably costs on the hotter paths (16 threads, warmed): | | unsynced | synced | |---|---|---| | `newDocumentBuilder()` | 68.1 µs/op | 115.6 µs/op | | `newSAXParser()` | 61.2 µs/op | 100.8 µs/op | That is ~1.7x worse under contention, on paths hit once per XML part parsed — far more often than `newTransformer()`, which runs once per part that has relationships on save. So the asymmetry in this PR is deliberate: lock the one the spec warns about and that is called rarely, leave the two that the implementations handle safely and that are called constantly. I have pushed a commit replacing the inaccurate comment with this reasoning so it does not have to be re-derived later. Happy to revisit if you would rather have uniform treatment. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
