Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid creation of SAXParserFactory for every read operation in Jaxb2Marshaller and co #32851

Closed
sturose opened this issue May 20, 2024 · 2 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@sturose
Copy link

sturose commented May 20, 2024

	private Source processSource(Source source) {
		if (StaxUtils.isStaxSource(source) || source instanceof DOMSource) {
			return source;
		}

		XMLReader xmlReader = null;
		InputSource inputSource = null;

		if (source instanceof SAXSource saxSource) {
			xmlReader = saxSource.getXMLReader();
			inputSource = saxSource.getInputSource();
		}
		else if (source instanceof StreamSource streamSource) {
			if (streamSource.getInputStream() != null) {
				inputSource = new InputSource(streamSource.getInputStream());
			}
			else if (streamSource.getReader() != null) {
				inputSource = new InputSource(streamSource.getReader());
			}
			else {
				inputSource = new InputSource(streamSource.getSystemId());
			}
		}

		try {
			if (xmlReader == null) {
				SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
				saxParserFactory.setNamespaceAware(true);
				saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());  //EXPENSIVE
				String name = "http://xml.org/sax/features/external-general-entities";
				saxParserFactory.setFeature(name, isProcessExternalEntities());  //EXPENSIVE
				SAXParser saxParser = saxParserFactory.newSAXParser();
				xmlReader = saxParser.getXMLReader();
			}
			if (!isProcessExternalEntities()) {
				xmlReader.setEntityResolver(NO_OP_ENTITY_RESOLVER);
			}
			return new SAXSource(xmlReader, inputSource);
		}
		catch (SAXException | ParserConfigurationException ex) {
			logger.info("Processing of external entities could not be disabled", ex);
			return source;
		}
	}

This is the current code. But creating a new SAXParserverFactory for every processSource is expensive. Calling setFeature can often result in code that then goes off and looks for resource files, which often are not there, until finally picking a default.

See https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/javax/xml/parsers/SAXParserFactory.html#newInstance()

However, (as stated in supplied link) Once an application has obtained a reference to a SAXParserFactory, it can be used to obtain parser instances.

It seems like the code ought to be:

        private SAXParserFactory saxParserFactory = null;

	private Source processSource(Source source) {
		if (StaxUtils.isStaxSource(source) || source instanceof DOMSource) {
			return source;
		}

		XMLReader xmlReader = null;
		InputSource inputSource = null;

		if (source instanceof SAXSource saxSource) {
			xmlReader = saxSource.getXMLReader();
			inputSource = saxSource.getInputSource();
		}
		else if (source instanceof StreamSource streamSource) {
			if (streamSource.getInputStream() != null) {
				inputSource = new InputSource(streamSource.getInputStream());
			}
			else if (streamSource.getReader() != null) {
				inputSource = new InputSource(streamSource.getReader());
			}
			else {
				inputSource = new InputSource(streamSource.getSystemId());
			}
		}

		try {
			if (xmlReader == null) {
			      if(saxParserFactory == null ) {
				  saxParserFactory = SAXParserFactory.newInstance();
				  saxParserFactory.setNamespaceAware(true);
				  saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
				  String name = "http://xml.org/sax/features/external-general-entities";
				  saxParserFactory.setFeature(name, isProcessExternalEntities());
                                }
				SAXParser saxParser = saxParserFactory.newSAXParser();
				xmlReader = saxParser.getXMLReader();
			}
			if (!isProcessExternalEntities()) {
				xmlReader.setEntityResolver(NO_OP_ENTITY_RESOLVER);
			}
			return new SAXSource(xmlReader, inputSource);
		}
		catch (SAXException | ParserConfigurationException ex) {
			logger.info("Processing of external entities could not be disabled", ex);
			return source;
		}
	}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 20, 2024
@sturose
Copy link
Author

sturose commented May 20, 2024

The same pattern should also be applied to Jaxb2RootElementHttpMessageConverter as it is doing the same thing.

@jhoeller jhoeller added in: data Issues in data modules (jdbc, orm, oxm, tx) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 20, 2024
@jhoeller jhoeller self-assigned this May 20, 2024
@jhoeller jhoeller added this to the 6.1.8 milestone May 20, 2024
@jhoeller
Copy link
Contributor

We have a couple of places where we are creating a SAXParserFactory on the fly, as a consequence of #27239. We need to revise this to cache the SAXParserFactory instance in all applicable places.

@jhoeller jhoeller changed the title Jaxb2Marshaller creating SAXParserFactory for every processSource Avoid creation of SAXParserFactory for every read operation in Jaxb2Marshaller and co May 21, 2024
@jhoeller jhoeller added the for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x label May 21, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels May 21, 2024
jhoeller added a commit that referenced this issue May 21, 2024
Includes JAXBContext locking revision (avoiding synchronization) and consistent treatment of DocumentBuilderFactory (in terms of caching as well as locking).

Closes gh-32851

(cherry picked from commit a4c2f29)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants