Skip to content

Make use of languages' thread enter/exit more correct#96760

Merged
akien-mga merged 2 commits into
godotengine:masterfrom
RandomShaper:wtp_langs_exit_thread
Sep 11, 2024
Merged

Make use of languages' thread enter/exit more correct#96760
akien-mga merged 2 commits into
godotengine:masterfrom
RandomShaper:wtp_langs_exit_thread

Conversation

@RandomShaper

@RandomShaper RandomShaper commented Sep 9, 2024

Copy link
Copy Markdown
Member

To ensure ScriptLanguage::thread_exit() for WorkerThreadPool's thread is called, the lifetime of the pool needed to be slightly modified so the threads reach an end before the language server is terminated.

This PR does so, and also does it in a way that the lifetime of the singleton is managed more consistently with that of other basic ones.

Fixes #95809.

@CedNaru (poster of the issue hopefully fixed), would you test this, please?

UPDATE:
Added a resilience commit, with several benefits:

  • The language server will bookkeep itself if a thread has been entered/exited for scripting.
  • It will gracefully handle redundant requests, so WorkerThreadPool and Thread can't keep it simple. WTP still needs to have the late enter logic, but now it's possible to do so in an optimal way.

@RandomShaper RandomShaper added bug topic:core cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 9, 2024
@RandomShaper RandomShaper added this to the 4.4 milestone Sep 9, 2024
@RandomShaper RandomShaper requested a review from a team as a code owner September 9, 2024 16:20
@CedNaru

CedNaru commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

@RandomShaper
I locally cherry-picked this branch (+ fixed its small current error) and built our custom 4.3 engine.
I can confirm it works as intended. I created tasks for the WorkerThreadPool when running a local project to force a thread_enter and, when closing the engine, thread_exit was properly called before the ScriptServer ends.

However, I have one concern with those changes. It seems that if a given thread of the pool is not used during runtime, thread_enter will never be called on it as it's done right before processing a task, but thread_exit will always be called when the pool is terminated. It means that a thread can have its exit callback triggered without the enter callback. Our custom language implementation is safe against such thing, but it might not be the case for everyone.

@RandomShaper RandomShaper changed the title WorkerThreadPool: Enhance lifetime for more flexibility Make use of languages' thread enter/exit more correct Sep 10, 2024
@RandomShaper

Copy link
Copy Markdown
Member Author

That's correct. I was assuming all thte languages would be robust enough to handle that. However, you've encouraged me to improve the state of affairs, so I've added something else (see the shortly updated PR description).

Also, thanks for testing, and, well, please test this again.

@CedNaru

CedNaru commented Sep 10, 2024

Copy link
Copy Markdown
Contributor

It works like a charm, thank you.
The ScriptServer doing the bookkeeping is certainly a better design, it keeps the responsibility in the single place.

Now I have a question that feels like a corner case, but it's still a doubt I have. From what I understand of the recent changes regarding the WorkThreadPool and CommandQueue, the Godot servers' main functions are now running in a looping task inside the pool instead of their own thread. Those servers are started before the ScriptServer, which include their looping task. thread_enter is called only when a thread is about to process a new task, but it doesn't seem to include the ones already running.
Unless I understand something wrong, it seems that the thread of the pool running the RenderingServer (for example) will never call thread_enter. During my test, "thread_enter" was only called by threads outside the pool or by the pool thread processing a task I manually queued, but it never happened for the remaining pool threads.

Godot has a number of server methods that take a Callable for different kind of callback (a recent example being the new compositor callback in the RenderingServer). Doesn't that mean that a Callable bound to a script can be called by the server thread without a previous call to "thread_enter", as it's technically still the same task running from before ScriptServer was initialized?

@RandomShaper

Copy link
Copy Markdown
Member Author

@CedNaru, you're right again. I've updated this PR to include coverage for such situations. I've realized I could also simplify the checks and just call ScriptServer::thread_enter() as the revised implementation already does the optimized (lock free, just TLS bool check) first.

@RandomShaper

Copy link
Copy Markdown
Member Author

And, well, if you could test again, I'd be hugely grateful.

@CedNaru

CedNaru commented Sep 10, 2024

Copy link
Copy Markdown
Contributor

Tested again. And my issue is still fixed with those last changes.

I can't think of other cases that could cause more issues. From what I understand, every time a worker thread yields, it's going to call thread_enter, meaning that only one "tick" of the server function is now necessary to call the ScriptServer once that one is initialized. That should give us plenty of time in practice.

I'm okay merging that PR as it is.

@akien-mga akien-mga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been well tested and I trust @RandomShaper with the implementation.

@RandomShaper

Copy link
Copy Markdown
Member Author

For the records, there's still something we haven't handled well yet neither before nor after this PR: languages added and/or removed dynamically.

@CedNaru

CedNaru commented Sep 10, 2024

Copy link
Copy Markdown
Contributor

Currently the Kotlin binding is a module just like C# so it's not an issue for now, but we plan to move to an extension in a few months. I guess language extensions fall into the dynamic category (Well, I don't see what else it could be).

@akien-mga akien-mga merged commit 658b8a8 into godotengine:master Sep 11, 2024
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

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

Labels

bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WorkerThreadPool doesn't call ScriptServer::thread_exit()

3 participants