-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Make use of languages' thread enter/exit more correct #96760
Make use of languages' thread enter/exit more correct #96760
Conversation
@RandomShaper 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. |
9c217e6
to
2a86da4
Compare
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. |
It works like a charm, thank you. 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. 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? |
2a86da4
to
c8acf56
Compare
@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 |
And, well, if you could test again, I'd be hugely grateful. |
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 I'm okay merging that PR as it is. |
There was a problem hiding this comment.
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.
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. |
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). |
Thanks! |
To ensure
ScriptLanguage::thread_exit()
forWorkerThreadPool
'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:
WorkerThreadPool
andThread
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.