Thread::wait_to_finish rename to Thread::join; add detach method; correct warning/error

[Jimmio92]

Not sure if I'm doing this right; this is my first attempt at a pull request/GitHub.

This changes Thread::wait_to_finish to be the more standard and shorter Thread::join; this is why it touches some 45 files. Maybe not the time to be changing this -- but I really don't see why we'd avoid the standardized naming conventions. Joining a thread implies a wait.

This adds a detach method (and documentation) to Thread. Detached threads continue processing, cleaning up when they return (C++ level, then OS/kernel level). This allows you to "fire-and-forget" a thread, letting it signal when it's done, returning the data then automatically cleaning up.

There was a warning about having to call wait_to_finish, but I can't see it doing any cleanup, really. I moved that "cleanup" code to the destructor in case of some template/macro artifact I may be missing because I am unfamiliar with the engine source, but I doubt it's necessary. The warning was removed.

There was an error that could occur, as a detached thread is likely still alive, just "disconnected" so to speak from the main thread. This meant I shouldn't cause that possible template/macro artifact and in doing so I added a SafeFlag joinable; it is used internally and not exposed to the end user in GDScript. This variable can likely be worked out of the equation if I give it more time.

Not in this pull request though related: I'd be willing to rewrite the threading stuff in an OS-specific manner. Doing so can greatly improve the speed of certain things (for example, using a critical section on Windows platforms is often much faster than a kernel-level mutex -- no ring jump), and can grant us the ability to cancel/kill threads we have created, which as it stands the C++ standard library does not provide a std::thread::cancel.

[Jimmio92]

Ah... I forgot to change the tests it seems. Oops. I'll try to do a more thorough search tomorrow for uses of wait_to_finish when I have free time.

[bruvzg]

I'd be willing to rewrite the threading stuff in an OS-specific manner.

FYI: At some point, it was implemented as platform specific way, but we replaced it with standard C++ implementation for various reasons - see #45618.

[RandomShaper]

This PR goes the opposite direction to the recent history of the engine in several ways. I don't think we want to change heading back. We consider that C++ standard threading is more beneficial (less code to maintain), even if it has downsides. From the performance perspective, it's true that Windows critical sections have advantages, but that's why we have thread pools and may introduce a similar mechanism for script users. As of today, users are advised not to rely on threads being created fast, but having them created upfront. Regarding the warning, it's for encouraging best practices. I can't say a detach or fire-and-forget dynamics don't have any value, but that may warrant a proposal in the proposals repo for thorough discussion.

[Jimmio92]

Ah crud, I must have forgotten to list it. There's a bug this fixes, #71048. There's a proposal I created as well to further discuss this at godotengine/godot-proposals#6062.

[Jimmio92]

This PR goes the opposite direction to the recent history of the engine in several ways. I don't think we want to change heading back. We consider that C++ standard threading is more beneficial (less code to maintain), even if it has downsides. From the performance perspective, it's true that Windows critical sections have advantages, but that's why we have thread pools and may introduce a similar mechanism for script users. As of today, users are advised not to rely on threads being created fast, but having them created upfront. Regarding the warning, it's for encouraging best practices. I can't say a detach or fire-and-forget dynamics don't have any value, but that may warrant a proposal in the proposals repo for thorough discussion.

Not sure if you missed what I've done here. I did not implement OS native threads -- only said I'd be willing to do it. I renamed wait_to_finish to join, and added a detach method. That's all I've done. This fixes a bug that recently popped up.

[RandomShaper]

Sure, I was just saying that for your information. Regarding the proposal, sorry. I had missed it.

Now, could you share a minimal reproduction project showing what you are trying to achieve and why the warning is pointless?

[Jimmio92]

I seem to have missed something vital in doing this as I'm now having issues with my own code. closing pull request for now.

[Jimmio92]

The threading stuff is still broken in Godot 4.0RC6. This pull request is a good first step to solving it -- but after bad interactions with folks in the community, I'm just going to reopen this and let someone else deal with getting the tests working (I think that's all I missed). I don't have time unfortunately, and definitely don't have time for the drama.