-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix inconsistent behavior when zend-max-execution-timers is enabled #19786
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
...when zend-max-execution-timers is enabled.
From what I understand, this happens because zend_timeout_handler() installs the EG(hard_timeout) handler, but zend_interrupt_helper() immediately calls zend_timeout(), which calls zend_set_timeout_ex(0, 1);. This clears the installed hard timeout and makes the shutdown procedure loop forever.
I wonder whether we should call zend_set_timeout_ex(0, 1); in zend_timeout() at all? Maybe you know, @arnaud-lb? Removing it also resolves the issue, though your patch likely also makes sense to unify the different timers.
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 reasonable as-is, awaiting Arnaud's answer for my question. 🙂
@iluuu1994 I'm not sure, but I think that calling zend_set_timeout_ex(0, 1) is necessary because the signal handler is set with SA_RESETHAND in some cases, so it needs to be reinstalled after it fires.
The proposed change looks right to me, and is consistent with other timeout implementations.
Thank you @Appla!
... build Only enable for 8.3 because of GH-19786.
When
zend-max-execution-timersis enabled, this test case consistently fails.This patch aligns its behavior with other types of timeout implementations.
I'm unsure whether related code paths—such as main.c#L2523 and php_cli_server.c#L2278—should also be updated.