Fix ApnsPHP_Push to not send a pcnt_signal_dispatch if is not in serverMode

[jbaez]

If running standalone ApnsPHP_Push in a custom cli script that uses pcntl signals, sending APNS notifications will affect the proper function of it.

[lucabrunox]

It makes sense, but can you explain what's the problem with pcntl_signal_dispatch in your custom script? Also consider this change might break existing code that relies on that dispatch, so I'd rather let the user choose whether or not to dispatch signals instead of disabling them unconditionally.

[jbaez]

For what I understand from the code the pcntl_signal_dispatch is used in ApnsPHP_Push::send method because is necessary when it is used by the ApnsPHP_Push_Server subclass. Checking if the function exists is not the best approach to execute pcntl_signal_dispatch since you might be using ApnsPHP_Push in a CLI script that uses pcntl signals for other purposes. People shouldn't be relying in pcntl signals when using ApnsPHP_Push since that is a standalone class to send APNS notifications which requirements are: if you plan to use only Push and Feedback provider without the Server part you need only OpenSSL (no PCNTL, System V shared memory or semaphore)

[lucabrunox]

@jbaez ok, but can you explain your specific problem? What's your use case for which the dispatch is wrong.

[jbaez]

Yes, we have a script that uses pcntl_fork to create child processes which run beanstalk jobs among other things. The pcntl_signal_dispatch was causing child processes to be killed.

[lucabrunox]

@jbaez oh I guess that's because you are queuing the kill signal on purpose? Thus pcntl_signal_dispatch would make the child quit?

[jbaez]

@lethalman the worker CLI script is running as a "daemon" doing a pcntl_signal_dispatch in a loop. It looks that the pcntl_signal_dispatch in the child it's causing a SIGCHLD to be sent to the worker which then will kill the child.

[lucabrunox]

@jbaez if you get a SIGCHLD it's because the child has exited, and with pcntl_signal_dispatch you see it. pcntl_signal_dispatch per se shouldn't send a SIGCHLD to the parent, rather the parent gets it because the child has actually terminated.

Can you debug more the situation? I think pcntl_signal_dispatch is actually catching a problem in your app.

[jbaez]

@lethalman Well I debugged it, the problem was in the child job that involved sending APNS messages which uses this library, and pcntl_signal_dispatch was causing the problem. The bug doesn't occur all the time, normally I have to leave it running for a day to occur which makes it very difficult to debug. When it happens beanstalk (we are sending APNS messages using beanstalk) enters in an endless loop trying to send 2 APNS messages. They never finish as the SIGCHLD is killing the job before it's finished. This fix in the library solved the problem.

[lucabrunox]

@jbaez it's not a "fix", it's a fix for your program that you applied to this library. I'm sure existing applications are relying on apns triggering signals in the loop.

I could accept a different patch: add an option to disable the dispatch. That is, by default it's on also for non-server mode, but the user can choose to disable it, though I don't see any reason for disabling it.

[jbaez]

@lethalman It is ok with me you don't want to accept the patch, but in my opinion there is no need to use pcntl_signal_dispatch in this class. I don't think people using this class are expecting it to send pcntl_signal_dispatch signals (which is also not documented) and it's only intention is to work with ApnsPHP_Push_Server subclass. If someone using the library would like ApnsPHP_Push to send pcntl_signal_dispatch for any reason they just have to subclass it and in the constructor set $this->_bServerMode = true; Anyway, thanks for getting involved and try to suggest fixes, as you said a simpler way of avoid it sending signals is to add a public setter in ApnsPHP_Push class to modify the serverMode flag, I just chose not to because I don't see the reason why it should be sending signals.

[lucabrunox]

@jbaez the reason for dispatching signals is that's a busy loop, and it would block signals if you don't dispatch them. It's quite natural for scripts, whether you are using the server class or your own customized server class. So the dispatch must be there in any normal case.

Subclassing just to set a flag is inconvenient, I'd rather have an explicit setDispatchSignals.

[jbaez]

@lethalman Ok then, I will add a commit to the pull request using the setter instead when I get some free time.