Use ITIMER_REAL for timeout handling on Apple Silicon systems
#13567
Conversation
|
Can someone explain me, why this PR is failing? I cannot conclude why my change can have impact on |
|
it does not, don t worry :) |
Zend/zend_execute_API.c
Outdated
| @@ -1522,7 +1522,8 @@ static void zend_set_timeout_ex(zend_long seconds, bool reset_signals) /* {{{ */ | |||
| t_r.it_value.tv_sec = seconds; | |||
| t_r.it_value.tv_usec = t_r.it_interval.tv_sec = t_r.it_interval.tv_usec = 0; | |||
|
|
|||
| # if defined(__CYGWIN__) || defined(__PASE__) | |||
| # if defined(__CYGWIN__) || defined(__PASE__) || (defined(__aarch64__) && defined(__APPLE__)) | |||
| // ITIMER_PROF is broken in Apple Silicon system with MacOS >= 14 | |||
There was a problem hiding this comment.
PHP uses tabs, not spaces. (using the refined-github extensin allows to see that kind of issues during code reviews)
There was a problem hiding this comment.
Thank you, I will check this out.
|
It seems @arnaud-lb is looking into this now (#13592) |
|
Oh, I wasn't aware of this PR. I will close mine. |
|
Shouldn't this change also |
|
Sorry for the late reply, had the flue last week. |
| # if defined(__CYGWIN__) || defined(__PASE__) | ||
| # if defined(__CYGWIN__) || defined(__PASE__) || (defined(__aarch64__) && defined(__APPLE__)) | ||
| // ITIMER_PROF is broken in Apple Silicon system with MacOS >= 14. The SIGPROF signal is sent way too early | ||
| // when the process opens sockets. | ||
| setitimer(ITIMER_REAL, &t_r, NULL); | ||
| } | ||
| signo = SIGALRM; |
There was a problem hiding this comment.
SIGALRM may conflict with sleep(). I don't have Mac and can't check this.
There was a problem hiding this comment.
What do you have in mind exactly? I can check it on my machine. Do you think sleep is just broken?
There was a problem hiding this comment.
To prevent any conflict, for instance with pcntl_alarm(), you could use SIGIO, as in #13468 (comment).
There was a problem hiding this comment.
I meant calls to sleep() may conflict with signal handlers for SIGALARM.
This is the old well known problem. (see man 3 sleep - "On some systems, sleep() may be implemented using alarm(2) and SIGALRM (POSIX.1 permits this); mixing calls to alarm(2) and sleep() is a bad idea.")
I can't check this PR, and I can't take a decision about its acceptance or rejection.
There was a problem hiding this comment.
@dstogov according to the MacOS man page, sleep() does not use SIGALARM on this platform:
This function is implemented using nanosleep(2) by pausing for seconds seconds or until a signal occurs. Consequently, in this implementation, sleeping has no effect on the state of process timers, and there is no special handling for SIGALRM.
There was a problem hiding this comment.
I can confirm @arnaud-lb. This is what the man-page says.
|
Did this patch work for you @windaishi ? Still facing the issue with this patch, althrough error is a bit different:
Usually it gives the file path and line but not with this patch for some reason. |
|
@Herz3h this specific error message occurs when the script takes too long to shutdown after a timeout. Do you confirm the message is printed too early? Also, did you apply the two changes in this PR? |
|
@Herz3h yes it did. |
windaishi
changed the title
ITIMER_REAL for timeout handling on Apple Silicon systemITIMER_REAL for timeout handling on Apple Silicon systems
|
Thank you @windaishi ! |
|
Hi, Has this been merged in with 8.3? I'm still experiencing this issue with 8.3 but not with 8.2. Thanks Jonathan |
|
According to Github, it's available in 8.3.9RC1 and 8.2.21RC1
|
This closes #12814