Deadlock potential in LSP RequestManager (and it's usages in rename & quickfix implementations)

It's currently possible to induce a deadlock in the RequestManager by doing the following:

• Read Request [R1] is queued, then executed. RequestManager.queue is empty again and RequestManager.parallel is executing the Runnable of [R1].
• Write Request [W] is queued, then executed. The implementation of WriteRequest awaits the completion of all current Requests (which were also marked as cancelled). During this time it blocks RequestManager.queue.
• [R1] now starts another Read Request [R2] and waits for its completion.
• [R2] is now queued, but has to wait for [W] to complete, which waits for [R1] to complete, which waits for [R2] to complete. Hence deadlock.

I think this behaviour would actually theoretically be acceptable, though I'd explicitly document, that you may not join the resulting futures unless you can guarantee that you're not already in a Request.

The issue is, that this exact situation can already happen with the way it's being used in the default XText implementations.

Specifically I found it for the Rename request and in the ChangeConverter2, which is used for quickfixes. Here is the code paths explained:

ChangeConverter2

This path is probably the worse one (and the one we actually ran into with our quickfixes, since some of them take quite long to calculate.

The ChangeConverter2 uses ILanguageServerAccess#doRead to access the document and then immediately awaits it:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/rename/ChangeConverter2.java#L80-L96

ILanguageServerAccess#doRead does start another Read Request:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L1145-L1151

However, this code already potentially runs in a request. E.g. in the default diagnostics path:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/editor/quickfix/DiagnosticResolution.java#L122-L144

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/codeActions/QuickFixCodeActionService.java#L80-L97

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L802-L828

Rename Deadlock

The code path for a rename request looks as follows:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L994-L1016

Here the requestManager is immediately called and a read request is queued. Inside of that read request, we execute the RenameService2:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/rename/RenameService2.java#L89-L150

Here line 95 and 146 are the issue. They queue a new read request and then await it. So if a write request is queued in the (admitedly short) time frame of the start of the request until the rename service queues its read request, you'll get a dead lock.

(the same applies for PrepareRename)

Suggested fix

ILanguageServerAccess already offers a doSyncRead method, which is identical to the doRead method, with the exception that it does NOT queue a new Read Request:

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L1192-L1196

vs

https://github.com/eclipse-xtext/xtext/blob/b6db004e1eff276d5ea2df982ff64623dfab033a/org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L1146-L1151

By using that method, we can now prevent a deadlock.

For the Rename path, this should be quite obviously fine (as it's guaranteed to run inside a read request already, so it doesn't interfere with the scheduling done via the RequestManager).

For the ChangeConverter2 it's less obvious for me. In the quickfix path, it should be required, but I am not sure if there are other code paths where the asynchronous request is necessary.
I can't say I understand why this was ever done in an asynchronous manner in the first place, tbh. I haven't come up with a scenario that would require or desire that.
Maybe you still know the reasoning behind it, and whether it's fine to also change it to a synchronous method?

Start with RequestManager and the doRead/doSyncRead implementations in LanguageServerImpl, then trace the linked ChangeConverter2, RenameService2, and quickfix paths. Verify how nested reads interact with queued writes, and confirm that rename, prepare-rename, and quickfix flows no longer create the described wait cycle while preserving the intended request semantics.