[core_bind] Add Thread::is_alive. Replace is_active with is_started to align with core/os/Thread API.

[briansemrau]

This adds the function is_alive to Thread. This is a usability improvement allowing users to check if a thread is still doing work. This helps resolve confusion around is_active (renamed to is_started), which was reasonably expected to behave as is_alive.

The current workaround to this is to create a finished_executing flag for each thread, set to false before thread.start and set to true at the end of the threaded function.

---

• I believe this fixes Thread remains in active state when function has finished #7235
• Addresses part of Improve Thread for better usability godot-proposals#3163

If/when this gets approved, I'll open a branch for 3.x that doesn't break compatibility.

[akien-mga]

I think if it's implemented in core_bind::Thread, it should also be implemented in Thread for consistency. CC @RandomShaper

[briansemrau]

I think if it's implemented in core_bind::Thread, it should also be implemented in Thread for consistency.

If you're referring to core/os/thread, then is_active doesn't exist there either. Regardless, if it's decided that they should be consistent then I can look into that.

Though, it appears that core/os Thread::is_started should behave the same as core_bind Thread::is_active, but they have different implementations. Not sure why it was done that way.

[akien-mga]

Though, it appears that core/os Thread::is_started should behave the same as core_bind Thread::is_active, but they have different implementations. Not sure why it was done that way.

I think that was an oversight when core/os Thread was refactored in #45315. I ran into this (and other related core_bind discrepancies when trying to port a Godot module to the new GDExtension (which use the core_bind APIs for core classes): godotengine/godot-cpp#633

[RandomShaper]

• Bindings' Thread::is_active() could be reimplemented just as a call-through to C++'s Thread::is_started(). And it could also be renamed so both are called the same (probably is_started() would be the best; see below).
• Regarding having an is_executing() method, a couple of thoughts:
  1. I'd like to suggest naming it is_alive(), to match what Java and Python do (that's why is_started() would be better than is_active(), so there's a bigger difference between both words).
  2. It would be good to stress in the documentation the difference between is_started() and is_alive(). The former corresponds to the typical usage pattern of starting a thread and joining when you know it will end. The latter would be for cases when that's not possible or convenient and you'd rather poll (for instance, every frame).

[briansemrau]

@RandomShaper Do you think implementing is_alive in core/os/Thread is desired, creating parity with core_bind Thread::is_alive? If so, core/os/Thread must be used with reference counting (inherit RefCounted if even possible, otherwise refactor all thread usage to use shared_ptr?). This is to prevent undefined behavior when resetting the is_alive flag (p_self->running.clear();) if the thread gets detached. Detaching the thread is incorrect usage and prints a warning, but I'd prefer not to create threaded, undefined behavior under any circumstance.

I'm heavily against doing this because of the complexity, but perhaps there's a simple solution I'm missing. Also, as far as I know, this doesn't currently have a use in core.

[RandomShaper]

I agree with you; probably too involved to be worthy when the C++ side has been good without it so far.

[RandomShaper]

The only potential concern now is the exotic looking commit message, because of the square bracketed tag. Let's hear @akien-mga's opinion on that.

[akien-mga]

The only potential concern now is the exotic looking commit message, because of the square bracketed tag.

Exotic indeed in our codebase, but we don't have a fixed convention and it does convey important information here (C++ core vs bindings) so I guess it's fine as is.

[akien-mga]

Thanks!

[briansemrau]

I'll put a little more consideration into commit names in the future 😅