Repository navigation
Xpp3DomBuilder.createXmlReader: remove thread-unsafe system property manipulation - #393
Conversation
There was a problem hiding this comment.
Pull request overview
Removes thread-unsafe manipulation of the global org.xml.sax.driver system property from Xpp3DomBuilder.createXmlReader(), addressing a race condition in multi-threaded environments (e.g., Maven parallel builds) as described in issue #392.
Changes:
- Removed clear/restore of
org.xml.sax.driverfromcreateXmlReader()and now directly falls back toXMLReaderFactory.createXMLReader(). - Added a regression test asserting that
Xpp3DomBuilder.build(...)does not modifyorg.xml.sax.driver.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/maven/shared/utils/xml/Xpp3DomBuilder.java | Removes thread-unsafe global system property manipulation when creating an XMLReader. |
| src/test/java/org/apache/maven/shared/utils/xml/Xpp3DomBuilderTest.java | Adds a regression test to ensure build() does not change the SAX driver system property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
build() must not touch org.xml.sax.driver, which is what this test asserts. If that ever regresses, the test would leave the mutated value behind and take unrelated tests down with it. Restoring in a finally keeps the blast radius to this test, matching how OsTest handles system properties.
|
Pushed two commits to this branch: merged current The test now restores No change to Verified: This comment was created with AI assistance. |
slachiewicz
left a comment
slachiewicz
left a comment
There was a problem hiding this comment.
The race is real and removing the clear/restore is the right fix: a thread reading org.xml.sax.driver during another thread's window saw it cleared, and no restore ordering can make that safe.
Two corrections to the description, for the record. On JDK 17 and 25 the direct com.sun.org.apache.xerces.internal.parsers.SAXParser instantiation fails with IllegalAccessException, because java.xml does not export that package, so XMLReaderFactory.createXMLReader() is the path actually taken there rather than a rarely used fallback. The fix still holds: the factory returns the same Xerces parser when the property is unset. The one behaviour change is that a user-set org.xml.sax.driver is now honoured, including a broken value, where the old code silently ignored it. That is what the property is documented to do.
The new test also passes on the old code, since with the property unset the old clear and restore were no-ops. It guards against a future regression rather than demonstrating this one.
Squash-merging, since the branch carries a master merge and two fixups.
Verified: probe on JDK 17 and 25 → direct instantiation throws IllegalAccessException, factory returns com.sun.org.apache.xerces.internal.parsers.SAXParser; CI green on all 12 jobs.
This comment was created with AI assistance.
Xpp3DomBuilder.createXmlReader()clears and restores the globalorg.xml.sax.driversystem property without synchronization (lines 119-129). In a multi-threaded environment like Maven parallel builds, one thread's clear/restore sequence interferes with other threads' reads of the same property. The code's own comment acknowledges: "There's a 'slight' problem with this an parallel maven: It does not work ;)"The method already tries to directly instantiate
com.sun.org.apache.xerces.internal.parsers.SAXParserfirst — which succeeds on all Oracle/OpenJDK JVMs. The system property manipulation fallback is both unnecessary and harmful.Fix: Removed the system property manipulation entirely. If the direct instantiation fails, falls through to
XMLReaderFactory.createXMLReader()without modifying any global state.Fixes #392