Skip to content

Use ITIMER_REAL for timeout handling on Apple Silicon systems #13567

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Use ITIMER_REAL for timeout handling on Apple Silicon systems #13567

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
221de26
Use ITIMER_REAL for timeout handling on Apple Silicon system
windaishi Mar 1, 2024
7c7a3a7
Update Zend/zend_execute_API.c
windaishi Mar 2, 2024
07e0275
Add review feedback
windaishi Mar 11, 2024
bf99e20
Improve comment
windaishi Mar 11, 2024
88f3a1d
Update Zend/zend_execute_API.c
windaishi Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1522,7 +1522,9 @@ 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. The SIGPROF signal is sent way too early
// when the process opens sockets.
setitimer(ITIMER_REAL, &t_r, NULL);
}
signo = SIGALRM;
Comment on lines -1525 to 1530
Copy link
Member

Choose a reason for hiding this comment

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

SIGALRM may conflict with sleep(). I don't have Mac and can't check 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.

What do you have in mind exactly? I can check it on my machine. Do you think sleep is just broken?

Copy link
Member

@dunglas dunglas Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To prevent any conflict, for instance with pcntl_alarm(), you could use SIGIO, as in #13468 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 can confirm @arnaud-lb. This is what the man-page says.

Expand Down Expand Up @@ -1586,7 +1588,7 @@ void zend_unset_timeout(void) /* {{{ */

no_timeout.it_value.tv_sec = no_timeout.it_value.tv_usec = no_timeout.it_interval.tv_sec = no_timeout.it_interval.tv_usec = 0;

# if defined(__CYGWIN__) || defined(__PASE__)
# if defined(__CYGWIN__) || defined(__PASE__) || (defined(__aarch64__) && defined(__APPLE__))
setitimer(ITIMER_REAL, &no_timeout, NULL);
# else
setitimer(ITIMER_PROF, &no_timeout, NULL);
Expand Down