Always use Zend signal handling #5710

alexdowad · 2020-06-13T14:25:35Z

Follow up to PR #5591.

Instead of eliminating Zend signal handling altogether, simplify things in a different way; always enable Zend SH, thus eliminating the --{disable,enable}-zend-signals configuration parameters.

This global variable controls whether the Zend deferred signal handlers are installed at the beginning of each request or not. Interestingly, it is set to 1 on initialization, and there is only *one* place where the value is modified. This is in OPCache, where the global is temporarily set to 0 while preloading. Why is this? Discussion on GitHub revealed that the purpose was so that what happens during preloading would not affect subsequent requests... but the fact is, as long as Zend signal handling is enabled, the Zend signal handlers will always be installed on each subsequent request, so installing them during preloading will not affect anything.

KalleZ · 2020-06-13T14:32:46Z

Since Zend Signal Handling was never available on Windows, these checks should most likely be changed to ZEND_WIN32, TSRM_WIN32 or PHP_WIN32 checks in their relevant places to keep it disabled.

Edit: Note that Windows does have some signal functionality available, but it is not used, so you may have to explicitly disable it for Wiindows.

alexdowad · 2020-06-13T15:08:49Z

@KalleZ Thank you for the review!

Correct me if I'm wrong, but I don't think the existing code does anything to explicitly disable Zend SH on Windows -- it's just disabled by virtue of the fact that Windows doesn't have sigaction. As such, I believe that this simplified code is correct as written.

Is that not so? Looking forward to further feedback.

nikic · 2020-06-13T15:17:14Z

There's a build failure on AppVeyor:

Zend\zend.c(168): error C2065: 'zend_signal_globals_t': undeclared identifier
Zend\zend.c(168): error C2059: syntax error: ')'
Zend\zend.c(169): error C2059: syntax error: ','
Zend\zend.c(170): error C2059: syntax error: '}'

alexdowad · 2020-06-13T15:18:29Z

There's a build failure on AppVeyor:

Zend\zend.c(168): error C2065: 'zend_signal_globals_t': undeclared identifier
Zend\zend.c(168): error C2059: syntax error: ')'
Zend\zend.c(169): error C2059: syntax error: ','
Zend\zend.c(170): error C2059: syntax error: '}'

Just fixing it up. Sorry!

KalleZ · 2020-06-13T15:22:26Z

@KalleZ Thank you for the review!

Correct me if I'm wrong, but I don't think the existing code does anything to explicitly disable Zend SH on Windows -- it's just disabled by virtue of the fact that Windows doesn't have sigaction. As such, I believe that this simplified code is correct as written.

Is that not so? Looking forward to further feedback.

The issue is that Windows does not define ZEND_SIGNALS, with the removal of the macros it is blindly assumed that zend signals is always available, which will enable zend signal handling for Windows (and cause your AppVeyor build error). Please keep it disabled here, unless you can actively test on Windows to make sure it works as intended (in which case I don't think AppVeyor test running is enough).

alexdowad · 2020-06-13T15:32:29Z

The issue is that Windows does not define ZEND_SIGNALS, with the removal of the macros it is blindly assumed that zend signals is always available, which will enable zend signal handling for Windows (and cause your AppVeyor build error). Please keep it disabled here, unless you can actively test on Windows to make sure it works as intended (in which case I don't think AppVeyor test running is enough).

Thanks for commenting again. But it's actually the opposite; the AppVeyor build logs show that the code in the bottom part of zend_signal.h, which is used when HAVE_SIGACTION is false, is being compiled on Windows.

There is another problem with the build, which I am trying to fix.

Being able to choose whether Zend signal handling should be used or not does not benefit users (or packagers) of PHP in any way. The use of Zend SH is a purely internal implementation detail, so there is no need for the build config parameters --{disable,enable}-zend-signals. By just choosing one way to do things (in this case, always using Zend signal handling), we can simplify the codebase a bit. Of course, some platforms do not have the concept of signals and signal handlers at all. That is why previously, Zend signal handling was automatically disabled if the platform did not have `sigaction`. In harmony with that, we now guard the implementation of Zend signal handling with `#ifdef HAVE_SIGACTION`.

alexdowad · 2020-06-13T17:14:34Z

It's fixed now. Sorry about that. 😓

KalleZ · 2020-06-13T19:37:17Z

Well whether or not it compiles on Windows is one thing, it needs to be stable if we are going to enable it. There was some mentions of this in the RFC that implemented the Zend signal handling, more specifically it mentions that ZTS support was at the time yet to be implemented for Windows. The "Improved stability" section near the top also made an observation for some SAPIs with signals on Windows.

https://wiki.php.net/rfc/zendsignals#considerations

alexdowad · 2020-06-13T19:47:27Z

Well whether or not it compiles on Windows is one thing, it needs to be stable if we are going to enable it. There was some mentions of this in the RFC that implemented the Zend signal handling, more specifically it mentions that ZTS support was at the time yet to be implemented for Windows. The "Improved stability" section near the top also made an observation for some SAPIs with signals on Windows.

https://wiki.php.net/rfc/zendsignals#considerations

I'm sorry if what I said was not clear. My point was that with this patch, Zend signal handling is already disabled in Windows. It is "automatically" disabled for exactly the same reason as was the case before this patch -- Windows does not have sigaction.

If you look at the existing code already on master, the build scripts do not do any check to see if we are building on Windows when enabling/disabling Zend signal handling. They simply check if sigaction is present on the platform, and if not, Zend SH is disabled.

With this patch, it works in just the same way. There is no change in that regard.

Hope this is making sense now.

alexdowad · 2020-06-13T19:57:28Z

@KalleZ Sorry that I seem to be struggling to get my point across.

Please have a look at the existing code for configuring Zend SH:

php-src/Zend/Zend.m4

Lines 306 to 320 in 63cb47a

    
           AC_ARG_ENABLE([zend-signals], 
        
             [AS_HELP_STRING([--disable-zend-signals], 
        
               [whether to enable zend signal handling])], 
        
             [ZEND_SIGNALS=$enableval], 
        
             [ZEND_SIGNALS=yes]) 
        
           AC_CHECK_FUNCS([sigaction], [], [ 
        
             ZEND_SIGNALS=no 
        
           ]) 
        
           if test "$ZEND_SIGNALS" = "yes"; then 
        
           	AC_DEFINE(ZEND_SIGNALS, 1, [Use zend signal handling]) 
        
           	CFLAGS="$CFLAGS -DZEND_SIGNALS" 
        
           fi 
        
           AC_MSG_CHECKING(whether to enable zend signal handling)

You will see that there is no mention of WIN32 there. However, look at lines 312-314... that is what currently causes Zend SH to be disabled on Windows.

This patch uses the very same criteria to ensure that Zend SH is disabled on Windows. See https://github.com/php/php-src/pull/5710/files#diff-ea48211bef574717fa8ad58ed9c829eeR39.

paresy · 2020-06-25T21:16:04Z

Could you maybe have a look here? https://bugs.php.net/bug.php?id=79464

Signaling on ZTS seems to be broken fundamentally (due to threads being used when signals are per process defined)

Maybe it would be the right time to fix this as well?

For my embed ZTS builds zend signals are always disabled. Having them enabled had some unwanted side effects as far as i remember.

alexdowad · 2020-06-26T06:30:52Z

Could you maybe have a look here? https://bugs.php.net/bug.php?id=79464

Signaling on ZTS seems to be broken fundamentally (due to threads being used when signals are per process defined)

Maybe it would be the right time to fix this as well?

For my embed ZTS builds zend signals are always disabled. Having them enabled had some unwanted side effects as far as i remember.

I have a fix already prepared for the issue of execution timeouts not working on ZTS, but am waiting for the issue of Zend signal handling to be worked out first...

Here I had proposed to remove Zend signal handling: #5591. That was not accepted, so I opened this PR to always turn it on instead. My view is that there is no reason to have a compile-time config parameter for Zend SH: if it is needed, it should always be on. If it is not needed, it should be removed.

paresy · 2020-06-26T08:57:32Z

I added my comment in the other PR. I would prefer to have Zend Signals removed because in my use case i they do not play well with threading. When this PR get's merged it would need to patch them out again ;)

Do you have a branch where i could try the other fix?

velemas · 2020-07-20T07:09:54Z

Hi from much less popular mainframe OS BS2000 (namely its POSIX subsystem). I second paresy. On recent php 7.4.8 zend signals break a lot of tests on BS2000 (when disabled ~600 tests pass additionally). Moreover with zend signals some phpdbg tests loop infinitely. I truss'ed one of them and the loop was:

getpid()->kill(SIGSEGV)->sigprocmask()->sigaction()->getpid()->...

I think it is very difficult if not impossible to implement universal zend signals which behave correctly on all kinds of signal system implementation. So please leave it at least configurable (--disable-zend-signals).

paresy · 2020-08-11T12:44:23Z

Could you maybe have a look here? https://bugs.php.net/bug.php?id=79464
Signaling on ZTS seems to be broken fundamentally (due to threads being used when signals are per process defined)
Maybe it would be the right time to fix this as well?
For my embed ZTS builds zend signals are always disabled. Having them enabled had some unwanted side effects as far as i remember.

I have a fix already prepared for the issue of execution timeouts not working on ZTS, but am waiting for the issue of Zend signal handling to be worked out first...

@alexdowad: You mentioned you had a fix "ready" for the mentioned bug report? Would you be able to share it somewhere?

Also i'd be interested which would be the proper way to proceed with your two PR's? I'd be in favor of closing this one and merging the one to remove Zend SH. As far as i have seen in the other PR the only question was about a performance analysis requested by dstogov.

alexdowad · 2020-08-12T20:08:06Z

Dear @paresy, thanks for your interest in my work. You can have a look at https://github.com/alexdowad/php-src/tree/refactor-timeout if you like.

That code is a WIP, but was getting close to being ready to merge. Some commits would be squashed first, etc. It works in CI, and I have confirmed that on Mac OS, the libdispatch code is actually being used.

Also i'd be interested which would be the proper way to proceed with your two PR's?

Ask the maintainers.

dstogov · 2023-10-09T16:41:39Z

The PR looks outdated.
I'm closing it now.
Please update an re-open if it makes sense.
Personally, I see sense in removing a possibility to make a non-tested PHP build, but I'm not sure if this costs the effort.

alexdowad force-pushed the always_use_zend_signals branch from b93f902 to fb1f674 Compare June 13, 2020 15:06

alexdowad force-pushed the always_use_zend_signals branch from fb1f674 to be55ff1 Compare June 13, 2020 15:23

alexdowad force-pushed the always_use_zend_signals branch from be55ff1 to 1fc5f3d Compare June 13, 2020 15:40

alexdowad requested review from dstogov, iluuu1994 and bukka as code owners October 7, 2023 13:51

dstogov closed this Oct 9, 2023

Always use Zend signal handling #5710

Always use Zend signal handling #5710

Uh oh!

Conversation

alexdowad commented Jun 13, 2020

Uh oh!

KalleZ commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

nikic commented Jun 13, 2020

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

KalleZ commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

KalleZ commented Jun 13, 2020

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

alexdowad commented Jun 13, 2020

Uh oh!

paresy commented Jun 25, 2020

Uh oh!

alexdowad commented Jun 26, 2020

Uh oh!

paresy commented Jun 26, 2020

Uh oh!

velemas commented Jul 20, 2020

Uh oh!

paresy commented Aug 11, 2020

Uh oh!

alexdowad commented Aug 12, 2020

Uh oh!

dstogov commented Oct 9, 2023

Uh oh!

Uh oh!

KalleZ commented Jun 13, 2020 •

edited

Loading

KalleZ commented Jun 13, 2020 •

edited

Loading