-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat: macOS support for Zend Max Execution timers #13468
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
base: master
Are you sure you want to change the base?
Conversation
I tried with an NTS build and it works, so this is ready for review. What I haven't handled yet is hard timeouts (that have never been supported for ZTS builds anyway). |
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.
This looks good to me appart from a few comments. I tested on Monterey/Intel under apache with and without ZTS.
However my knowledge of MacOS is limited so I would like to see other reviews. @devnexen WDYT?
Zend/zend_max_execution_timer.c
Outdated
pthread_kill(*tid, ZEND_MAX_EXECUTION_TIMERS_SIGNAL); | ||
#else | ||
pid_t *pid = (pid_t *) arg; | ||
kill(*pid, ZEND_MAX_EXECUTION_TIMERS_SIGNAL); |
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 this can cause zend_timeout_handler
to be executed in parallel to the VM: We have multiple threads due to Dispatch or other libraries starting helper threads, so the signal may be delivered in any of them.
This probably already happens anyway, and zend_timeout_handler
is threads safe for the most part (probably not the hard timeout part).
We could call zend_timeout_handler
directly here for the same effect.
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'm trying. The red tests may be related to this.
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.
This seems to work, but tests are still red.
dispatch_time(DISPATCH_TIME_NOW, seconds * NSEC_PER_SEC), | ||
seconds * NSEC_PER_SEC, | ||
0 |
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.
Can we disable recurrence of the timer?
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.
No, this seems not possible: https://developer.apple.com/documentation/dispatch/1385606-dispatch_source_set_timer
Nice work overall ,but I ll have a better look either later today or within this weekend with my sonoma/arm machine. |
return; | ||
} | ||
|
||
dispatch_queue_global_t queue = dispatch_get_global_queue(QOS_CLASS_UTILITY, 0); |
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.
quick question: did you try with using your own queue or at least, does it make any meaningful difference in your opinion e.g.
dispatch_queue_attr_t attr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, QOS_CLASS_UTILITY, 0);
dispatch_queue_t queue = dispatch_queue_create("net.php.zend_max_execution_timer", attr);
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.
According to the docs, this queue looks adapted to our use case, but I'm not a specialist in Mac specifics.
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.
ACK. Was wondering what is best for, at least, ZTS context.
d3b1fbb
to
54b4daf
Compare
Just an update here that I have been trying to figure out why FPM tests are failing which we were discussing internally. The problem seems to be that curl initialization uses internally through another library (possibly krb5) libdispatch which happens in master process before forking. When the timer is later activated in the child process after fork through I created this simplified program that reproduces the crash: #include <dispatch/dispatch.h>
#include <curl/curl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/wait.h>
#include <unistd.h>
#include <signal.h>
void timer_handler(void *ctx)
{
printf("handle\n");
}
void timer_cancel(void *ctx)
{
printf("cancel\n");
}
void sigchld_handler(int signum) {
int status;
pid_t pid;
while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
if (WIFEXITED(status)) {
printf("Child %d exited with status %d\n", pid, WEXITSTATUS(status));
} else if (WIFSIGNALED(status)) {
printf("Child %d killed by signal %d\n", pid, WTERMSIG(status));
}
}
}
int main()
{
// if global curl init in parent, it crashes
curl_global_init(CURL_GLOBAL_DEFAULT);
signal(SIGCHLD, sigchld_handler);
int pid = fork();
if (pid == 0) {
// if global init in the child, it works
// curl_global_init(CURL_GLOBAL_DEFAULT);
dispatch_queue_global_t queue = dispatch_get_global_queue(QOS_CLASS_UTILITY, 0);
dispatch_source_t timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, queue);
if (timer == NULL) {
printf("timer is null\n");
return 1;
}
dispatch_source_set_event_handler_f(timer, timer_handler);
dispatch_source_set_cancel_handler_f(timer, timer_cancel);
dispatch_source_set_timer(
timer,
dispatch_time(DISPATCH_TIME_NOW, 2 * NSEC_PER_SEC),
2 * NSEC_PER_SEC,
0
);
dispatch_activate(timer);
sleep(10);
} else if (pid > 0) {
printf("created child %d\n", pid);
int status;
waitpid(pid, &status, 0);
if (WIFEXITED(status)) {
printf("Child exited with status %d\n", WEXITSTATUS(status));
} else if (WIFSIGNALED(status)) {
printf("Child killed by signal %d\n", WTERMSIG(status));
}
printf("finishing\n");
} else {
printf("fork error\n");
}
return 0;
} Not sure what we can do about it. It will require some further investigation. |
Just for the record I created the issue for libdispatch. Arnaud found out the exact place in Curl so the reproducer there is a bit updated. I might be looking to possibility of introducing some child hook in FPM which we could potentially use for Curl initialization if we don't find a better solution. |
Co-authored-by: Peter Kokot <[email protected]>
Add support for the new timeout system (#10141) on macOS.
Closes #12814.
Relies on Grand Central Dispatch.
I tested the patch with FrankenPHP (ZTS build) and it seems to work.