Avoid warning when catching signal with pcntl_signal()

[Divi]

Creating a temporary error handler when selecting/accepting a stream to avoid warning messages, caused by catching a signal with function pcntl_signal().

See issue : #296

[romainneutron]

Symfony's implementation is quite similar but more precise I think. See implementation at https://github.com/symfony/symfony/blob/27e1a893a090bf58bce774660f6cb5cbdfd4d287/src/Symfony/Component/Process/ProcessPipes.php#L359-L365 and usage at https://github.com/symfony/symfony/blob/27e1a893a090bf58bce774660f6cb5cbdfd4d287/src/Symfony/Component/Process/ProcessPipes.php#L235-L241

[Divi]

Great idea, but they use the prefix @, and I wanted to avoid it due to low performance of this trick.

[romainneutron]

I'm curious about what low performance you're speaking. Symfony's code bypasses the error and check if it was a interrupted system call only if stream_select returned false whereas your implementation switches the error handler twice for every call. It's definitely not a such low performance trick :)

[Divi]

I was not saying my fix is more efficient than another. I just tried to avoid the @ trigger.
Moreover, I didn't know about the false return statement on error of the function stream_select, so the Symfony's fix is clearly better.
I can't fix it this PR today. I'll do the change tomorrow :)

[romainneutron]

no worries. Avoid the use of @ when it's about dicarding errors when they should not be, but in this case we just filter errors that could be safely discarded.
thanks for your work

[Divi]

Fixes are available as @romainneutron advised. I saw a bug only after some commit sorry.

[mathielen]

Works like a charm. When does it get merged back?

[clue]

Some tests would be much appreciated 👍

Also IIRC, the error messages take the user's LOCALE into account and might be translated to another language. If so, how should we match those?

[Divi]

@clue I've my locale set to french and the message is still in english.

[kingcrunch]

When is it planned to get merged?

[POPSuL]

Why not merged yet?

[cboden]

This is now a read-only repo made up of components loaded from composer. Merge requests need to made against the event-loop. In addition, I wouldn't want to include @stream_select in the source code. stream_select is the core of the EventLoop and error suppression is detrimental to performance.

[mkrauser]

Right, supressing errors is bad for performance, especially inside the event-loop. I wouldn't put this into the core either. But there should be a way out for users. Maybe a special subclass or at least a hint in the docs.

Regarding the check for the return-value: this should definitely be merged. If the stream_select-call fails, you should not rely on the references.

[HarasimowiczKamil]

Try use declare(ticks=1); on top in root file. Then you can catch signals without errors/warnings.

[frodeborli]

I am pretty sure this will slow down your app considerably.

On Wed, Jun 10, 2015 at 7:51 AM, Kamil Harasimowicz <
notifications@github.com> wrote:

Try use declare(ticks=1); on top in root file. Then you can catch signals
without errors/warnings.

—
Reply to this email directly or view it on GitHub
#297 (comment).

[HarasimowiczKamil]

Probably yes but in many cases it might not be as critical, and will work without problems.

[clue]

This PR seems like a good starting point, thanks @Divi!

This PR has been WIP for quite some time now and things have changed considerably since then :-)

React now consists of individual components that are maintained individually. Supporting signal handling is now related to the react/event-loop component.

Somebody has already filed a ticket reactphp/event-loop#8 to keep track of this, so I suppose it makes sense to focus our signal handling related efforts on this component.