Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes LSP connection error when launched in a separate thread #80686

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

azuloo
Copy link
Contributor

@azuloo azuloo commented Aug 16, 2023

Fixes #79525 . I went with set_current_thread_safe_for_nodes(true) and it fixes the problem but I'm not sure if it doesn't introduce any security issues. But from what I've found out - GDScriptLanguageServer has no thread processing (current_process_thread_group = nullptr) and here we're left with two choices:

  • only access nodes that are not part of a scene tree (that's not happening in out case) and
  • mark a current thread as 'safe' (which is what this PR does)

Please, let me know if I'm missing some other possible ways of fixing this.

@RandomShaper
Copy link
Member

This is good enough, but maybe not as good as it should in the long term. Let me explain.

If the LSP was working well, silencing the errors (what this PR does) will not make it any worse, but taking it back to the level of functionality it used to enjoy. In fact, the idea with set_current_thread_safe_for_nodes() was to also give a shortcut in cases like this to at least keep them working as they used to do without adding the obligation of a deep analysis of threading correctness right away.

Now, is this an opportunity to realize actual threading issues that warrant additional work? I can't answer this myself due to my lack of familiarity with what the LSP does.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 21, 2023
@akien-mga akien-mga merged commit e43370d into godotengine:master Aug 21, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 21, 2023
@azuloo azuloo deleted the lsp-thread-connection-error branch August 21, 2023 19:09
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSP: Thread error when connecting to language server with VScode
5 participants