RequestManager modifies ResourceSet in parallel

[mark-christiaens]

The org.eclipse.xtext.ide.server.concurrent.RequestManager is executing read and write requests. AFAICT, the read operations are executed on the parallel executor service. The write requests are executed by the queue executor. So, the net result is that read requests can occur in parallel. However, as we've seen multiple times in the past, the Resources and ResourceSets are not thread-safe. Even when only performing read operations, EMF will update parts of the model. I cannot identify a synchronization mechanism that makes this code thread-safe.

I've seen IllegalStateExceptions being thrown like this:

Thread [pool-3-thread-10] (Suspended (breakpoint at line 137 in XtextResourceSet))
        XtextResourceSet.registerURI(Resource) line: 137
        XtextResourceSet$ResourcesList.inverseAdd(Resource, NotificationChain) line: 152
        XtextResourceSet$ResourcesList.inverseAdd(Object, NotificationChain) line: 146
        XtextResourceSet$ResourcesList(NotifyingListImpl<E>).addUnique(E) line: 286
        XtextResourceSet$ResourcesList(AbstractEList<E>).add(E) line: 305
        XtextResourceSet(ResourceSetImpl).createResource(URI, String) line: 435
        XtextResourceSet(ResourceSetImpl).demandCreateResource(URI) line: 243
        XtextResourceSet.getResource(URI, boolean) line: 260
        ProjectManager.xtend line: 134
        WorkspaceManager.xtend line: 214
        LanguageServerImpl.xtend line: 389
        LanguageServerImpl.xtend line: 379
        537290625.apply(Object) line: not available
        RequestManager.xtend line: 93
        1062050498.call() line: not available [local variables unavailable]
        FutureTask<V>.run() line: 266
        ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1149
        ThreadPoolExecutor$Worker.run() line: 624
        Thread.run() line: 748

These seem to be caused by a race on adding a new Resource to the ResourceSet.

I suspect that the "fix" is to make the execution of the read and write requests sequential (as is the case for the standard Xtext implementation).

[cdietrich]

i assume we need to distinguish which resources are read and what is done. sequencing the reads has performance impacts, but how to detect what kind of operation does modify (loading resources, resolving references, installing adapters is impossible imho

the queue itself is single threaded, but the work is then offloaded i also cannot relate the line numbers to anything.

@szarnekow @kittaakos do you have any idea why it is done this way? is the assumption that the resource is already loaded?

@mark-christiaens what is the service you execute?

[mark-christiaens]

@cdietrich I connected VS code to our LSP server and was editing, navigating, hovering ... The reason why I think that I triggered the behavior is that our LSP server is quite slow: I suspect that many of the read and write requests take several seconds a piece. So the window for "raciness" is wide.

With regards to performance: correctness trumps speed. I too have no idea how to assure that the underlying resource set is not mutated: the notion of "read only" access to EMF does not exist. Maybe @kittaakos can clarify the reasoning?

[cdietrich]

see also https://github.com/eclipse/xtext-core/issues/1135

[svenefftinge]

The underlying assumption is that with our standard hooks the resources are all loaded during indexing or a write operation. @mark-christiaens do you run a language that does some late lazy loading of dependencies? We could maybe introduce some property per language that tells the LS if parallel read is ok. So if one participating language does on read loading it could veto this.

[mark-christiaens]

@svenefftinge

• Yes, our language does perform late loading of additional resources that were not relevant during the main build.
• Besides that, IIUIC, one can choose the clustering policy (currently set to DisabledClusteringPolicy) so that from time to time part of the resource set is removed. That might cause load operations during following read or write request that consult the resource set.
• Is the basic architecture of LSP then such that all resources should be loaded at the same time? Would such a strategy scale to large projects?

[svenefftinge]

No, it would not scale, but it performs nicely if it fits into memory. Assuming that for most usages the fast (and simple) default strategy works well, I'd like to allow languages to opt-out of the parallel read by configuration. I understand that this is not super safe but IIRC it makes quite a difference to allow parallel read. So'd vote for going with that unsafe compromise (we have it for two years now)

[svenefftinge]

So what I am proposing is to change the manager to sequential read/write if a) one of the contributing language requires that b) the clustering policy is used.