Add TSRM free thread handlers

[realFlowControl]

Hey folks,

I needed to observe new thread initialisation and found (thanks to the help of @bwoebi) that there are tsrm_set_new_thread_begin_handler and tsrm_set_new_thread_end_handler functions that I could use. Now I see that there is no handler for when the thread gets freed (and joined), just a tsrm_set_shutdown_handler for when the TSRM shuts down.

This PR aim is to add the missing free thread handlers.

Alternative considered: I am working on an extension that does not have globals, so globals_ctor and globals_dtor won't be called, I could fake create globals that I do not use in order to get GINIT/GSHUTDOWN working, but that seems just wrong, although technically working 😉

[bwoebi]

Looks nice to me :-)

[Girgias]

This sounds sensible to me, but I don't really know TSRM

[nielsdos]

I would go for the GINIT/GSHUTDOWN approach honestly, I don't like that observers show up everywhere in the engine and cause troubles.

[realFlowControl]

That's what I'll need to do for older versions, but I can't shake the feeling that it's a workaround rather than a proper solution.

[..] I don't like that observers show up everywhere in the engine and cause troubles.

Is there something specific about this PR that raises that worry, or is it more of a general thing? I don't really know much about TSRM, I just noticed this missing piece and added it because it worked for my needs. However, there might be something I’m overlooking from a technical perspective, hence my question.

[nielsdos]

The TSRM is meant to be separate from the PHP core and the extensions. Observers are something that are currently integrated into the engine and I believe they should not bleed over into a transparent resource/threading layer.
You already have a working solution with GINIT/GSHUTDOWN. You may find that a workaround, but in my opinion this patch is also a workaround. You are modifying code to observe thread creation, but the TSRM code itself is adjacent from thread creation. You should add observation code to whatever causes the thread creation in the first place, not the TSRM.
Also, since you are both affiliated with Datadog, at least one other person familiar with TSRM should give their blessing over a patch like this because otherwise it becomes very easy for Datadog to just add whatever observer code they want into PHP.

[bwoebi]

@nielsdos I agree on the topic of Datadog, and that's why I haven't merged it right away.

I do however disagree that TSRM shall be separate. TSRM is the threading abstraction of PHP, so to observe threads, one shall observe the TSRM. There are more potential places (e.g. frankenphp uses TSRM directly) where threads are spawned, which are not hookable.

We personally don't exactly mind this not being accepted - after all, we do have a workaround. I just suggested doing it, for completeness as TSRM has a start handler, but somehow no end handler. And it is, I think, less hacky than abusing GSHUTDOWN (whose role is rather destroying globals than doing end-of-thread handling specifically).

[nielsdos]

I do however disagree that TSRM shall be separate. TSRM is the threading abstraction of PHP, so to observe threads, one shall observe the TSRM. There are more potential places (e.g. frankenphp uses TSRM directly) where threads are spawned, which are not hookable.

The thing is that neither the PHP core nor TSRM create the thread, it's the server that does it, which is part of why I feel weird about this.
You can interpose (on *nix) thread creation API, or use a hook on Windows.
Why exactly do you want to observe thread creation/shutdown?

[bwoebi]

Because we want to observe specifically threads used for PHP execution.

As to why: recording when a specific thread id stops being used.

[petk]

The TSRM is meant to be separate from the PHP core and the extensions.

I'm sorry for jumping in the middle of conversation here. Please let's also move TSRM already in the Zend engine and delete the TSRM directory. This was a plan for years and it got forgotten due to messy PHP organization.