Repository navigation
Conversation
…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
| } | ||
| if (thread_resources && !tsrm_thread_exit_registered) { | ||
| tsrm_thread_exit_registered = true; | ||
| _tlv_atexit(tsrm_thread_exit_handler, NULL); |
There was a problem hiding this comment.
Nit: Could this we moved next to the pthread_key_create() call? Purely for clarity / consistency
There was a problem hiding this comment.
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.
| free(data); | ||
| return; | ||
| } | ||
| if (FlsGetValue(tsrm_exit_key) != data) { |
There was a problem hiding this comment.
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?
| return; | ||
| } | ||
| if (FlsGetValue(tsrm_exit_key) != data) { | ||
| free(data); |
There was a problem hiding this comment.
I think that freeing data in this case is wrong
| do { | ||
| zend_try { | ||
| zend_destroy_rsrc_list(&EG(persistent_list)); | ||
| completed = true; | ||
| } zend_end_try(); | ||
| } while (!completed); |
There was a problem hiding this comment.
The catch/loop seem unrelated to the current PR?
There was a problem hiding this comment.
Hah I recall this one, had to add it because the persistent_list destructors can bailout.
There was a problem hiding this comment.
Could you split this out of this PR? Is this necessary at all? Not a fan of catching bailouts.
There was a problem hiding this comment.
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.
| #elif defined(__APPLE__) | ||
| tsrm_thread_exit_armed = pthread_key_create(&tsrm_exit_key, NULL) == 0; | ||
| #else | ||
| tsrm_thread_exit_armed = pthread_key_create(&tsrm_exit_key, tsrm_thread_exit_handler) == 0; |
There was a problem hiding this comment.
We should print something to stderr when these fail, and maybe abort()
There was a problem hiding this comment.
See comment below, it's deliberate (at least for now) because I'm not sure under what circumstances (if any) the setup could fail.
We currently don't have any such cleanup, so I prefer to keep the current behaviour if something goes wrong, rather than exiting the process. The stderr should be enough to draw attention to it.
| - 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(). |
There was a problem hiding this comment.
Is that still true? In which case are resources not released?
There was a problem hiding this comment.
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.
... to automatically call
ts_free_threadat thread exitSAPI'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