Skip to content
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

feat: macOS support for Zend Max Execution timers #13468

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Feb 21, 2024

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.

@dunglas
Copy link
Contributor Author

dunglas commented Feb 22, 2024

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).

Copy link
Member

@arnaud-lb arnaud-lb left a 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.h Outdated Show resolved Hide resolved
Zend/zend_execute_API.c Outdated Show resolved Hide resolved
pthread_kill(*tid, ZEND_MAX_EXECUTION_TIMERS_SIGNAL);
#else
pid_t *pid = (pid_t *) arg;
kill(*pid, ZEND_MAX_EXECUTION_TIMERS_SIGNAL);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Comment on lines +94 to +93
dispatch_time(DISPATCH_TIME_NOW, seconds * NSEC_PER_SEC),
seconds * NSEC_PER_SEC,
0
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zend/zend_globals.h Outdated Show resolved Hide resolved
Zend/Zend.m4 Show resolved Hide resolved
@devnexen
Copy link
Member

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);
Copy link
Member

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);

Copy link
Contributor Author

@dunglas dunglas Feb 25, 2024

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.

Copy link
Member

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.

@bukka
Copy link
Member

bukka commented Feb 27, 2024

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 dispatch_resume, it crashes.

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.

@bukka
Copy link
Member

bukka commented Feb 27, 2024

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.

Zend/Zend.m4 Outdated Show resolved Hide resolved
Zend/Zend.m4 Outdated Show resolved Hide resolved
Co-authored-by: Peter Kokot <peterkokot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max_execution_time reached too early
5 participants