Open ahmedneilhussain opened 3 weeks ago
FYI @angelozerr @rubenporras @ghentschke
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.
Stop press - it was indeed this issue that was causing our windows CI build to hang
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 @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
Yes, exactly.
Ironically I think this is caused by the fix for https://github.com/eclipse/tm4e/issues/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: https://github.com/eclipse/lsp4e/blob/11b81cfa0477fa9f8b135d0c9b3f8b45beb88640/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/semanticTokens/TokenTypeMapper.java#L45
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 callsbut it made no difference when I replaced
syncExec
withexecute
. 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?