-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Introduce new zend_module_entry hook: child_startup_func #13551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Starting threads in `request_startup_func` is unsafe because SAPIs may fork after this hook. Yet, some modules may start threads as part of their initialisation and have no easy way to safely do so. For example, the `curl` module calls `curl_global_init()` in `request_startup_func` to initialize `libcurl`, but this function may start threads at least on MacOS. This change introduces a new hook: `child_startup_func`. This hook is executed once per request-handling process, which are assumed to be non-forking processes. Modules should use this hook for their thread-creating initialization.
Would it make sense to instead have a zend_fork hook?
Which is used by fpm and pcntl_fork() both.
This would probably make pcntl_fork a tiny bit safer, as then extensions can just do some pre-fork cleanup as well, possibly synchronizing on some state to make it safer (like calling curl_global_cleanup too).
Unfortunately it's not possible to transition the process in a fork-safe state after using libdispatch, for instance. I suspect this will be the case for most libraries using threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I was also thinking maybe to introduce just some registration hook during MINIT but this is probably cleaner.
You might also need to update lightspeed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's not much point to force SAPI's to call child startup if they don't create any children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to let SAPI code to call it during startup if no child_startup defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but the separation of startup/child_startup happens to be useful even for these SAPIs in the context of opcache.preload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we no longer add /* }}} */ and /* {{{ */ to the new functions
Uh oh!
There was an error while loading. Please reload this page.
Starting threads in
module_startup_func(MINIT) is unsafe because SAPIs may fork after this hook. Yet, some modules may start threads as part of their initialisation and have no easy way to safely do so.For example, the
curlmodule callscurl_global_init()inrequest_startup_functo initializelibcurl, but this function starts threads at least on MacOS.This change introduces a new hook:
child_startup_func(CHINIT). This hook is executed once per request-handling process, which are assumed to be non-forking processes. Modules should use this hook for their thread-creating initialization.This fixes a crash observed in #13468, and follows an idea from @bukka.
NB: Forking a process with threads may leave child processes in a corrupted state and lead to crashes or deadlocks. In the case of #13468, curl was indirectly using
libdispatchduringMINIT, which created threads, and the library defensively crashed child processes trying to use it again, after that. This problem is well described here and here.NB2: opcache preloading may execute php code during module startup, in the main process. This requires calling the child startup hook, but this breaks the assumption that processes calling the child startup hook do not fork. It supports executing the preload script in a sub-process when preload_user is specified. In this case it's safe to call the child startup hook (in the sub-process). I've changed preloading so that the preload script is always executed in a sub-process. As a side-effect,
FFI::load()can never be used during preloading, andffi.preloadshould be used instead. Previously,FFI::load()could be used during preloading when fork wasn't needed.NB3: We can not guarantee that
pcntl_fork()will ever be safe