Crash on view closing during applying of semantic tokens

[rchl]

Describe the bug

It's possible to get an error like below. It reproduces fairly often when testing renaming of directories (with code from https://github.com/sublimelsp/LSP/pull/2498) when LSP opens new files, edits them and immediately closes.

Error handling None
Traceback (most recent call last):
  File "/Users/rafal/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/sessions.py", line 2564, in on_payload
    handler(result)
    ~~~~~~~^^^^^^^^
  File "/Users/rafal/Library/Application Support/Sublime Text/Packages/LSP/plugin/session_buffer.py", line 696, in _on_semantic_tokens_viewport_async
    self._on_semantic_tokens_async(response)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/Users/rafal/Library/Application Support/Sublime Text/Packages/LSP/plugin/session_buffer.py", line 693, in _on_semantic_tokens_async
    self._draw_semantic_tokens_async()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/rafal/Library/Application Support/Sublime Text/Packages/LSP/plugin/session_buffer.py", line 754, in _draw_semantic_tokens_async
    if token_general_style["source_line"] != token_type_style["source_line"] or \
       ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
KeyError: 'source_line'

The problem is that View can become invalid at any point when running code in an async thread so the only reliable way to fix it is to never use any View API that might fail in async thread.

Moving all this logic to main thread would fix it but of course it's not ideal if the logic is expensive to run.

[jwortmann]

The problem is that View can become invalid at any point when running code in an async thread so the only reliable way to fix it is to never use any View API that might fail in async thread.

Moving all this logic to main thread would fix it but of course it's not ideal if the logic is expensive to run.

Another option would be to put all code that runs on the async thread and uses view API methods into a try block. But I assume this would apply to most features in LSP, not only semantic tokens. Even something like

if view.is_valid():
    view.style_for_scope(...)

might fail if the view is still alive during the evaluation of the condition, but not anymore at the API call on next line.

In the problematic method here we have https://github.com/sublimelsp/LSP/blob/4c632ce90c714dc79cfe0f71432801aa649ee288/plugin/session_buffer.py#L715-L717 which I think should basically do the same, but apparently it is not enough when executed on the async thread.

For this particular case with semantic tokens we could think about moving the view.style_for_scope calls into the init method of SessionBuffer/SessionView, so that it is only evaluated a single time at the start and then the cached result is used. But that would be problematic if the user for example changes the color scheme or has just modified the styles for semantic token scopes in the color scheme.

[rchl]

Another option would be to put all code that runs on the async thread and uses view API methods into a try block. But I assume this would apply to most features in LSP, not only semantic tokens.

I'm strongly against sprinkling try/except blocks :) Also, the API itself doesn't cause exception in this case but rather returns None and some code later raises exception.

For this particular case with semantic tokens we could think about moving the view.style_for_scope calls into the init method of SessionBuffer/SessionView, so that it is only evaluated a single time at the start and then the cached result is used. But that would be problematic if the user for example changes the color scheme or has just modified the styles for semantic token scopes in the color scheme.

I would be fine with adding caching and sacrificing instant update on theme/style changes (which feels like a corner case to me). Could even bring real performance gains (not sure how expensive view.style_for_scope is).

[jwortmann]

I would be fine with adding caching and sacrificing instant update on theme/style changes (which feels like a corner case to me). Could even bring real performance gains (not sure how expensive view.style_for_scope is)

I would be okay with that. Then we should probably add an info box about that into the docs, so that users don't get confused if they adjust their color scheme for semantic highlighting but it doesn't work immediately. And even better would be to also listen for the select_color_scheme command to get notified when user changes the color scheme via the builtin color scheme picker, and then re-evaluate the token colors and redraw semantic highlighting.

[rchl]

I think it's fine to close this now since that specific error won't happen anymore.

There were various fixes that improved the situation - caching of the tokens, some extra is_valid() checks and some improvements in canceling requests so this is a lot less likely to trigger.

Feel free to reopen if you disagree.