Deadlock on UI thread from Semantic Tokens markup with recent TM4e

[ahmedneilhussain]

Ironically I think this is caused by the fix for eclipse/tm4e#721 (which was at the instigation of LSP4e). I am using eclipse 2023-09 but with tm4e 11 installed (I wanted some of the bug fixes in recent versions that haven't yet made it into a version of TM4e bundled with an eclipse release).

Please see the attached thread dump: you can see the main thread (also the UI thread) thinks the document is dirty and is waiting for stuff to be posted onto the queue at https://github.com/eclipse-platform/eclipse.platform.ui/blob/934ccc66e79042dccd94093376e403d9a932dda9/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/reconciler/AbstractReconciler.java#L123

Meanwhile the semantic token reconciler is trying to get the current theme:

lsp4e/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/semanticTokens/TokenTypeMapper.java

Line 45 in 11b81cf

return TMUIPlugin.getThemeManager().getDefaultTheme().getToken(tokenType);

and in tm4e after the fix for issue 721 above, this now will try and use the UI thread to find out if the current theme is dark.

Personally, I think it a bit dodgy for AbstractReconciler to have a method that blocks its caller, if its caller can be the UI thread, but if that's a thing that can happen, then I don't think reconciler threads should be calling syncExec(): it looks as though the reconciler mechanism is supposed to be purely headless background computation that just posts its results onto a queue for the UI to pick up, but I haven't investigated in detail.

This occurred every time in one of our SWTBot-driven integration tests once I switched over to using TM4e as well as LSP4e, but I think it might be very timing-sensitive. It was fine interactively, worked fine in CI on MacOS, but hung every time when I ran just that test locally on my development machine (also a Mac). Our CI is hanging on windows, which I haven't confirmed is the same issue, but I'm hoping that that's the case! Our test teardown logic calls

    PlatformUI.getWorkbench()
        .getDisplay()
        .syncExec(
            () ->
                PlatformUI.getWorkbench()
                    .getActiveWorkbenchWindow()
                    .getActivePage()
                    .closeAllEditors(false));

but it made no difference when I replaced syncExec with execute. I don't think this is an unreasonable thing to be calling. The fact this is MacOS and UI thread = main is a possible factor but I don't think so: fundamentally it looks like calling UI stuff on a reconciler thread is problematic.

Not sure what the correct fix is - it seems overkill to be looking up the theme once per token, but I guess this is not cached as a way of dealing with the default theme being changed?

[ahmedneilhussain]

hang2.txt

[ahmedneilhussain]

FYI @angelozerr @rubenporras @ghentschke

[ahmedneilhussain]

PS I'm using a (custom-shaded copy of) LSP4e 0.23, but I don't think that's the issue as TokenTypeMapper is unchanged.

[ahmedneilhussain]

Stop press - it was indeed this issue that was causing our windows CI build to hang

[rubenporras]

Hi @ahmedneilhussain ,

given that the code is only a fallback for the case where TM4E is not configured for the editor, I think we can just remove the fallback and just return null.

Would you like to do the PR?

Regards

[ahmedneilhussain]

Hi @ahmedneilhussain ,

given that the code is only a fallback for the case where TM4E is not configured for the editor, I think we can just remove the fallback and just return null.

Would you like to do the PR?

Regards

Hi - so I should just return null on line 45 and in the majority case it won't get that far, is that right?
If so, sure!

Ahmed

[rubenporras]

Yes, exactly.

[sebthom]

Is this issue solved?

[rubenporras]

I think so. @ahmedneilhussain , do you agree?

[sebthom]

Closing for now. Feel free to reopen if issue is not solved.