Parser.getDefault() is thread-unsafe and is used in multiple threads

[trancexpress]

Seen while debugging for: #666

E.g.:

	at org.eclipse.pde.internal.genericeditor.target.extension.model.UnitNode.setVersion(UnitNode.java:51)
	at org.eclipse.pde.internal.genericeditor.target.extension.model.xml.Parser.parse(Parser.java:74)
	at org.eclipse.pde.internal.genericeditor.target.extension.codemining.TargetDefinitionCodeMiningProvider.fillCodeMinings(TargetDefinitionCodeMiningProvider.java:52)
	at org.eclipse.pde.internal.genericeditor.target.extension.codemining.TargetDefinitionCodeMiningProvider.lambda$0(TargetDefinitionCodeMiningProvider.java:40)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1760)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188)

	at org.eclipse.pde.internal.genericeditor.target.extension.model.UnitNode.setVersion(UnitNode.java:51)
	at org.eclipse.pde.internal.genericeditor.target.extension.model.xml.Parser.parse(Parser.java:74)
	at org.eclipse.pde.internal.genericeditor.target.extension.validator.SyntaxValidatorListener.lambda$1(SyntaxValidatorListener.java:60)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1796)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188)

The code is respectively:

	@Override
	public CompletableFuture<List<? extends ICodeMining>> provideCodeMinings(ITextViewer viewer,
			IProgressMonitor monitor) {
		return CompletableFuture.supplyAsync(() -> {
			List<ICodeMining> minings = new ArrayList<>();
			IDocument document = viewer.getDocument();
			try {
				fillCodeMinings(document, minings);
			} catch (BadLocationException e) {
				// Caught with empty mining
			}
			return minings;
		});
	}

	void fillCodeMinings(IDocument document, List<ICodeMining> minings) throws BadLocationException {
		int line = 0;
		try {
			Parser parser = Parser.getDefault();
			parser.parse(document);
			Node target = parser.getRootNode();

		CompletableFuture.runAsync(() -> {
			try {
				if (fDocument.get().isEmpty()) {
					return;
				}
				Parser.getDefault().parse(fDocument);

Parser.default() does this:

	public static Parser getDefault() {
		if (instance == null) {
			instance = new Parser();
		}
		return instance;
	}

	public void parse(IDocument document) throws XMLStreamException {
		target = null;
		Node currentParent = null;
		Node currentNode = null;

	public Node getRootNode() {
		return target;
	}

Using getRootNode() is not safe in this case. UpdateUnitVersions also has similar to TargetDefinitionCodeMiningProvider code. Likely there is more such code.

In #666 we see that a task in parallel parsing potentially other documents can result in another task working with the wrong parsed contents (since it retrieves the wrong root note with Parser.getRootNode()).

[laeubi]

@trancexpress what do you think making the method just return a new parser instance always, I don't see any immediate benefit in caching it, nor any expensive operations in creating it.

[trancexpress]

@trancexpress what do you think making the method just return a new parser instance always, I don't see any immediate benefit in caching it, nor any expensive operations in creating it.

I guess we can try? Since parsing means I/O, I doubt creating the object costs much in comparison.

Maybe javax.xml.stream.FactoryFinder.find(Class<T>, String, ClassLoader, String) costs some, if there are many available implementations? Which is highly unlikely. Or the method itself could be slow due to looking for the implementations...

[merks]

You might be (likely will be) shocked to find out how expensive it is to create a parser. Please measure. It might easily be the dominant cost for processing small instances (like is the case for EMF).

[trancexpress]

What about returning the root node from the parse method? I.e. delete getRootNode(), I think that is the problem. The created/shared parser itself probably can stay the same. Or at least we have one less concurrency problem that way.

[laeubi]

@merks then one can still cache (static) XMLInputFactory apart from that "Parser" (bad chosen name here...) does not has any relevant fields.

What about returning the root node from the parse method? I.e. delete getRootNode()

Thats another option in wich case one might wonder why the whole method is not simply static

public class Parser {

	private static final XMLInputFactory inputFactory;

	static {
		inputFactory = XMLInputFactory.newInstance();
	}

	public static Node parse(IDocument document) throws XMLStreamException {
		Node currentParent = null;
		Node currentNode = null;
		Node target = null;
		Iterator<XMLElement> tagReaderIterator = createXMLTagItterator(document.get());
		while (tagReaderIterator.hasNext()) {
			XMLElement event = tagReaderIterator.next();
			if (event.isStartElement()) {
				String name = event.getName();
				if (ITargetConstants.UNIT_TAG.equalsIgnoreCase(name)) {
					UnitNode unit = new UnitNode();
					String unitValue = event.getAttributeValueByKey(ITargetConstants.UNIT_ID_ATTR);
					if (unitValue != null) {
						unit.setId(unitValue);
					}
					String versionValue = event.getAttributeValueByKey(ITargetConstants.UNIT_VERSION_ATTR);
					if (versionValue != null) {
						unit.setVersion(versionValue);
					}
					currentNode = unit;
				} else if (ITargetConstants.LOCATION_TAG.equalsIgnoreCase(name)) {
					currentNode = new LocationNode();
				} else if (ITargetConstants.DEPENDENCY_TAG.equalsIgnoreCase(name)) {
					currentNode = new DependencyNode();
				} else if (ITargetConstants.REPOSITORY_TAG.equalsIgnoreCase(name)) {
					currentNode = new Node();
					if (currentParent instanceof LocationNode containerLocation) {
						String locationValue = event.getAttributeValueByKey(ITargetConstants.REPOSITORY_LOCATION_ATTR);
						containerLocation.addRepositoryLocation(locationValue);
					}
				} else if (ITargetConstants.TARGET_TAG.equalsIgnoreCase(name)) {
					target = new Node();
					currentNode = target;
				} else {
					currentNode = new Node();
				}
				currentNode.setNodeTag(name);
				currentNode.setOffsetStart(event.getStartOffset());
				if (currentParent != null) {
					currentParent.addChildNode(currentNode);
				}
				currentParent = currentNode;
			}

			if (event.isEndElement()) {
				if (currentNode != null) {
					currentNode.setOffsetEnd(event.getEndOffset());
					currentNode = currentNode.getParentNode();
					currentParent = currentNode;
				}
			}
		}
		ByteArrayInputStream inputStream = new ByteArrayInputStream(document.get().getBytes());
		XMLEventReader eventReader = inputFactory.createXMLEventReader(inputStream);
		while (eventReader.hasNext()) {
			eventReader.nextEvent();
		}
		return target;
	}

	private static Iterator<XMLElement> createXMLTagItterator(String document) {
		return new Iterator<>() {
			private final String tagRegex = "(?<tag><[\\w|/][^<]+?>)";
			private final String commentRegex = "(<!--.*?-->)";
			private final String beforeTagRegex = ".*?(?=".concat(tagRegex).concat(")");

			private String text = document;

			private final Pattern tagPattern = Pattern.compile(tagRegex, Pattern.DOTALL);
			private final Pattern commentPattern = Pattern.compile(commentRegex, Pattern.DOTALL);
			private final Pattern beforeTagPattern = Pattern.compile(beforeTagRegex, Pattern.DOTALL);

			@Override
			public boolean hasNext() {
				skipComments();
				return text.length() > 0 && beforeTagPattern.matcher(text).find();
			}

			@Override
			public XMLElement next() {
				skipComments();
				text = beforeTagPattern.matcher(text).replaceFirst("");
				Matcher tag = tagPattern.matcher(text);
				tag.find();
				int offset = document.length() - text.length();
				text = text.substring(tag.end());
				return new XMLElement(tag.group("tag"), offset);
			}

			private void skipComments() {
				Matcher comment = commentPattern.matcher(text);
				Matcher tag = tagPattern.matcher(text);
				if (comment.find() && tag.find() && comment.start() < tag.start()) {
					text = text.substring(comment.end());
				}
			}
		};
	}

}

[trancexpress]

Seems like there is code that depends on whatever was parsed last:

Description	Resource	Path	Location	Type
The method getDefault() is undefined for the type Parser	AttributeValueCompletionProcessor.java	/org.eclipse.pde.genericeditor.extension/src/org/eclipse/pde/internal/genericeditor/target/extension/autocomplete/processors	line 58	Java Problem
The method getDefault() is undefined for the type Parser	TagCompletionProcessor.java	/org.eclipse.pde.genericeditor.extension/src/org/eclipse/pde/internal/genericeditor/target/extension/autocomplete/processors	line 92	Java Problem
The method getRootNode() is undefined for the type Parser	AttributeValueCompletionProcessor.java	/org.eclipse.pde.genericeditor.extension/src/org/eclipse/pde/internal/genericeditor/target/extension/autocomplete/processors	line 59	Java Problem
The method getRootNode() is undefined for the type Parser	TagCompletionProcessor.java	/org.eclipse.pde.genericeditor.extension/src/org/eclipse/pde/internal/genericeditor/target/extension/autocomplete/processors	line 93	Java Problem

E.g.:

	@Override
	public ICompletionProposal[] getCompletionProposals() {
		Parser parser = Parser.getDefault();
		Node rootNode = parser.getRootNode();
		if (rootNode == null) {
			return new ICompletionProposal[] {};
		}

No idea what to do with that.

I guess, leave things as are, but change Parser.parse to return also the root node it itself parsed? That way callers who care about what actually was done can rely on the result. Callers who just call a singleton and are "happy" with whatever state it has, have whatever was parsed last...

[trancexpress]

Lets discuss with actual changed code: #2139