TSRM: attach pthread_key/fls callbacks for thread cleanup by henderkes · Pull Request #24428 · php/php-src

henderkes · 2026-10-19T12:43:04Z

... to automatically call ts_free_thread at thread exit

SAPI's should already been doing this on their own before, but the apache2handler doesn't get control over threads, so it didn't call ts_free_thread and therefore started leaking resources with the move of per-thread globals from heap storage to native __thread storage

@shivammathur's requests in #23227
Will need minimal adjusting in the eg/cg move later, but it can already be merged standalone.

cc @arnaud-lb

…free_thread` at thread exit SAPI's should already been doing this on their own before, but the apache2handler doesn't get control over threads, so it didn't call ts_free_thread and therefore started leaking resources with the move of per-thread globals from heap storage to native __thread storage

arnaud-lb · 2026-10-19T12:55:03Z

+	}
+	if (thread_resources && !tsrm_thread_exit_registered) {
+		tsrm_thread_exit_registered = true;
+		_tlv_atexit(tsrm_thread_exit_handler, NULL);


Nit: Could this we moved next to the pthread_key_create() call? Purely for clarity / consistency

No, https://github.com/apple-oss-distributions/dyld/blob/main/include/mach-o/dyld.h#L104 documents that _tlv_atexit is for the calling thread, while the pthread_key_create is once for the process. We don't want to register for the main thread, but for the other threads spawned later.

arnaud-lb · 2026-10-19T13:04:49Z

+			return;
+		}
+		data->thread_resources = thread_resources;
+		if (!FlsSetValue(tsrm_exit_key, data)) {


Could we use FlsSetValue(tsrm_exit_key, thread_resources) directly? This would simplify the code substantially. If we can't, please add a comment to hint at why we use a separate struct.

Added a comment why we can't, we need some kind of identity mechanism because per-fiber (fls callbacks) is not per-thread (tsrm resources) and a fiber teardown could otherwise tear down a threads resources.

/* FLS is per-fiber, but TSRM resources are per-thread. Another fiber can free * and reallocate the resources while this fiber retains its FLS value. Keep a * separate record so callbacks can distinguish stale FLS values even if the * resource allocation reuses the same address. */

Let me know if I understand the concern here: We assume that ts_resource_ex() / allocate_new_resource() will be called from a thread, but that thread may be converted to a fiber later, which implies a few things:

The fiber may outlive out the thread and the related TLS

The fiber may be terminated before the thread

The fiber may execute or terminate on another thread

All of these cases will cause ts_free_thread to not be called, or to be called too early (2nd case).

Do we really want to support this? We should probably add some !IsThreadAFiber() assertions and ignore those cases.

Are there other "on thread terminate callback" mechanism we could use instead? Maybe we could use tss_create() or PIMAGE_TLS_CALLBACK?

arnaud-lb · 2026-10-19T13:57:06Z

+		free(data);
+		return;
+	}
+	if (FlsGetValue(tsrm_exit_key) != data) {


I may we wrong, but I think that we can simplify this to if (!data) {: If we are called because a Fiber exited, we receive that Fiber's specific data in arg, so it should be NULL?

But that's the wrong check, if !data would check if the thread or fiber doesn't have a value set. Here we want to compare if the value is the same one as that of our exit key. It could also be a fiber deleting another in which case data != NULL.

arnaud-lb · 2026-10-19T13:57:48Z

+		return;
+	}
+	if (FlsGetValue(tsrm_exit_key) != data) {
+		free(data);


I think that freeing data in this case is wrong

We're only freeing the fibers cleanup record here, not the actual resources (being data->thread_resources). Msdn says the callback is for freeing (that?) memory so I do believe it's correct.

(We allocate the cleanup record in tsrm_atexit_set_data())

arnaud-lb · 2026-10-19T14:12:17Z

+	do {
+		zend_try {
+			zend_destroy_rsrc_list(&EG(persistent_list));
+			completed = true;
+		} zend_end_try();
+	} while (!completed);


The catch/loop seem unrelated to the current PR?

Hah I recall this one, had to add it because the persistent_list destructors can bailout.

Could you split this out of this PR? Is this necessary at all? Not a fan of catching bailouts.

Could apply it later, but not before this PR, because persistent_list cleanup just doesn't happen on threads calling ts_free_thread on master now. I think it belongs in the PR, because it's really part of its handling.

arnaud-lb · 2026-10-19T14:26:26Z

+- Per-thread TSRM resources are now released best-effort at normal thread exit,
+  because native TLS resource destructors can only run on their owning thread and
+  tsrm_shutdown() cannot release them afterwards. Prefer explicitly calling ts_free_thread().


Is that still true? In which case are resources not released?

It's best-effort, pthread_key_create of FlsAlloc could fail and we don't want to fail php if a callback registration fails. Someone could also deliberately detach exit handlers before a thread exits... but yeah that's probably not worth mentioning.

henderkes · 2026-10-20T11:38:41Z

@arnaud-lb did the refactoring you asked for, I think for the other comments the code is right as is.

arnaud-lb · 2026-10-20T08:35:09Z

+	if (!tsrm_thread_exit_armed) {
+		fprintf(stderr, "TSRM: pthread_key_create() failed (error %d); automatic thread cleanup is disabled\n", error);
+	}


Can we return bool from this function and let the caller handle the logging?

A bool would miss the reason. A char* (buffer) maybe. But then we'd still assemble the string in the atexist function and the benefit is gone.

arnaud-lb · 2026-10-20T09:56:22Z

+			return;
+		}
+		data->thread_resources = thread_resources;
+		if (!FlsSetValue(tsrm_exit_key, data)) {


/* FLS is per-fiber, but TSRM resources are per-thread. Another fiber can free * and reallocate the resources while this fiber retains its FLS value. Keep a * separate record so callbacks can distinguish stale FLS values even if the * resource allocation reuses the same address. */

Let me know if I understand the concern here: We assume that ts_resource_ex() / allocate_new_resource() will be called from a thread, but that thread may be converted to a fiber later, which implies a few things:

The fiber may outlive out the thread and the related TLS

The fiber may be terminated before the thread

The fiber may execute or terminate on another thread

All of these cases will cause ts_free_thread to not be called, or to be called too early (2nd case).

Do we really want to support this? We should probably add some !IsThreadAFiber() assertions and ignore those cases.

Are there other "on thread terminate callback" mechanism we could use instead? Maybe we could use tss_create() or PIMAGE_TLS_CALLBACK?

henderkes · 2026-10-20T15:47:09Z

Let me know if I understand the concern here: We assume that ts_resource_ex() / allocate_new_resource() will be called from a thread, but that thread may be converted to a fiber later, which implies a few things:

The fiber may outlive out the thread and the related TLS

I'm not sure. Technically yes, but I don't think php, frankenphp or ext-parallel have any path causing it. Worst case I think cleanup would be missed, hence the recommendation to call ts_free_thread explicitly for embedders.

The fiber may be terminated before the thread [...] or to be called too early (2nd case).

Yes. [...] No, that's what we have the guard for that you asked about before. So it doesn't clean up too early.

The fiber may execute or terminate on another thread

Yes, see point 1.

Do we really want to support this? We should probably add some !IsThreadAFiber() assertions and ignore those cases.

Not particularly, but we don't want to crash when someone does silly things ;). If you want assertions (or, realistically, runtime checks, because whoever will use php downstream probably doesn't use a debug build), we can do that instead of the existing "best-effort" guards.

Are there other "on thread terminate callback" mechanism we could use instead? Maybe we could use tss_create() or PIMAGE_TLS_CALLBACK?

Afaik they run under DllMain locks and experience has taught me that you never want to do any sort of cleanup there.

arnaud-lb · 2026-10-20T16:00:20Z

Not particularly, but we don't want to crash when someone does silly things ;). If you want assertions (or, realistically, runtime checks, because whoever will use php downstream probably doesn't use a debug build), we can do that instead of the existing "best-effort" guards.

I would prefer that yes, because I'm having a very hard time understanding the current guards. This is adding a lot of complexity for an hypothetical use-case that we can't properly support anyway.

henderkes · 2026-10-20T16:18:31Z

Shut the pc down for the day, but sounds reasonable to me. @shivammathur I take it you have no immediate plans to use native fibers? 😆

shivammathur · 2026-10-20T16:29:13Z

Yes, no native Windows fiber requirement from my side. I agree with @arnaud-lb.

henderkes requested a review from dstogov as a code owner October 19, 2026 12:43

github-actions Bot added Category: Engine ABI break labels Oct 19, 2026

arnaud-lb reviewed Oct 19, 2026

View reviewed changes

henderkes added 3 commits October 19, 2026 16:56

suggestion: move FlsAlloc comment

fac1694

suggestion: refactor out platform specific atexit code

2085be1

suggestion: add stderr output in failure case

91991aa

github-actions Bot added the Category: Build System label Oct 20, 2026

arnaud-lb reviewed Oct 20, 2026

View reviewed changes

oopsie

7c371c0

Conversation

henderkes commented Oct 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes commented Oct 20, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes commented Oct 20, 2026

Uh oh!

arnaud-lb commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Oct 20, 2026

Uh oh!

shivammathur commented Oct 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henderkes Oct 20, 2026 •

edited

Loading

henderkes Oct 19, 2026 •

edited

Loading

arnaud-lb commented Oct 20, 2026 •

edited

Loading