eclipse.platform.ui icon indicating copy to clipboard operation
eclipse.platform.ui copied to clipboard

lsp4e/ModelServiceImpl: ConcurrentModificationException

Open jukzi opened this issue 1 year ago • 3 comments

found this is my log:

eclipse.buildId=4.34.0.I20240908-1800
java.version=21.0.2
java.vendor=Eclipse Adoptium
BootLoader constants: OS=win32, ARCH=x86_64, WS=win32, NL=de_DE
Command-line arguments:  -os win32 -ws win32 -arch x86_64

org.eclipse.lsp4e
Error
Tue Sep 10 10:41:21 CEST 2024
ConcurrentModificationException

java.util.ConcurrentModificationException
	at org.eclipse.emf.common.util.AbstractEList$EIterator.checkModCount(AbstractEList.java:751)
	at org.eclipse.emf.common.util.AbstractEList$EIterator.doNext(AbstractEList.java:699)
	at org.eclipse.emf.common.util.AbstractEList$EIterator.next(AbstractEList.java:685)
	at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElementsRecursive(ModelServiceImpl.java:277)
	at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElementsRecursive(ModelServiceImpl.java:288)
	at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElements(ModelServiceImpl.java:433)
	at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElements(ModelServiceImpl.java:414)
	at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElements(ModelServiceImpl.java:419)
	at org.eclipse.ui.internal.WorkbenchPage.getOrderedEditorReferences(WorkbenchPage.java:1061)
	at org.eclipse.ui.internal.WorkbenchPage.getEditorReferences(WorkbenchPage.java:2301)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:1024)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:1024)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at org.eclipse.lsp4e.LSPEclipseUtils.findOpenEditorsFor(LSPEclipseUtils.java:1525)
	at org.eclipse.lsp4e.operations.diagnostics.LSPDiagnosticsToMarkers.accept(LSPDiagnosticsToMarkers.java:93)
	at org.eclipse.lsp4e.operations.diagnostics.LSPDiagnosticsToMarkers.accept(LSPDiagnosticsToMarkers.java:1)
	at org.eclipse.lsp4e.LanguageClientImpl.publishDiagnostics(LanguageClientImpl.java:103)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$recursiveFindRpcMethods$0(GenericEndpoint.java:65)
	at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.notify(GenericEndpoint.java:160)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleNotification(RemoteEndpoint.java:231)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:198)
	at org.eclipse.lsp4e.LanguageServerWrapper.lambda$3(LanguageServerWrapper.java:314)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:185)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:97)
	at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:114)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)

clearly EObjectContainmentWithInverseEList is not thread safe. So the result of org.eclipse.e4.ui.model.application.ui.MElementContainer.getChildren() should not be used across threads. I don't know where this could be fixed: EMF / LSP / UI? @merks @mickaelistria ?

similiar error was already reported without lsp4e in https://bugs.eclipse.org/bugs/show_bug.cgi?id=507903

jukzi avatar Sep 10 '24 08:09 jukzi

It seems to me that no method of org.eclipse.ui.internal.WorkbenchPage is thread safe such that one can call it from a background thread. I see nothing to suggest there is any kind of synchronization nor locking in that class' methods nor in EModelService/ModelServiceImpl. Of course the UI might open and close editors while one is pawing through the same data structures in a background thread so that that's not safe. Furthermore, I don't think it should be made safe I expect any such effort would be pervasive and more likely to cause deadlocks than to solve problems. After all, the list of return editors might be immediately invalid right after the call....

I'm not sure what is being done with this list of editor references, but a syncExec would likely be more appropriate, or better to defer the final computation/processing to the UI thread via an asyncExec.

merks avatar Sep 10 '24 15:09 merks

possible improvements: a) writing a List copyConcurrentCopy(List) which copies the List - retrying on ConcurrentModificationException and using such copy to iterate over such a snapshot? b) use Display.syncCall in WorkbenchPage.getEditorReferences c) use Display.syncCall in LSPEclipseUtils.findOpenEditorsFor

jukzi avatar Sep 11 '24 07:09 jukzi

a) Changing the underlying model implementation, i.e., org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.findElementsRecursive, that paws through a tree of data structures all of which are implemented via an EMF model is pretty much a non-starter I think. b) WorkbenchPage should not promise thread safety nor be complicated by attempts to do so, in my opinion. c) This is more like what SWT expects, though with strong checks that ensure nothing unsafe is ever done on a non-display thread. This looks to me like the path forward and that needs to be done via an issue here:

https://github.com/eclipse/lsp4e/issues

I'm not sure an issue will be addressed there expect for self-service.

merks avatar Sep 11 '24 07:09 merks