check return value of stream_select including a test case

[cebe]

This PR contains a test case for #20 as requested by @clue and @WyriHaximus. I have also applied the fix by @mkrauser to make the tests pass.
This fixes the issue described in reactphp/reactphp#296.

The test sets up an event loop and then forks a child process which will send a signal to the current process while it is waiting for stream_select(). This triggers stream_select() to return false and throw a warning.

The test currently uses error_reporting to suppress this warning as in the issue discussion the supression of the warning with @ was no wanted. I think there should be a benchmark to check if adding an @ to stream select has any negative effect. I doubt it.

[cebe]

Additional note about adding @ to stream_select(): The "performance overhead" of suppressing the warning will hardly have any effect as stream_select() is supposed to be waiting some amount of time anyway in most cases. In the cases where it returns immediately, the reading and writing of streams will have much bigger impact than the very small amount of time added by @ operator.

[jmalloc]

Just in case the discussion around @ is what holds this up, I'd like to add this totally non-authoratative piece of information on the subject: https://github.com/ezzatron/fig-standards/blob/error-handler/proposed/error-handler-meta.md#431-performance-considerations

From my memory of the PHP source, @ simply temporarily set the error reporting level. Problems only arose when suppression of an error allowed failing code to continue execution of a loop with error logging enabled, i.e. with added I/O.

I guess the take-away from that would be to ensure you're not logging errors that indicate signal interrupts, as I can only assume that would be a blocking I/O operation.

[cebe]

added a commit for error suppression 5916176 commit message includes detailed resoning about why it should be added.

[cebe]

the test failure on travis is not caused by the changes btw.

[clue]

Changes LGTM Thanks for digging into this @cebe!

[clue]

FWIW: This is currently only tested for the StreamSelectLoop, does it make sense to test / document signal behavior for the other loops?

[cebe]

It would make sense of course but I'd do another PR after this one is merged. The scope of this PR is fixing an issue that only exists in stream select loop.

[cebe]

@clue @WyriHaximus can we get this one merged please, so I can start checking signal support for other event loops?

[cboden]

On one system I'm getting:

Fatal error: Call to undefined function React\Tests\EventLoop\pcntl_signal() in /Users/cboden/Repositories/React/event-loop/tests/StreamSelectLoopTest.php on line 107

Which can be fixed by checking extension_loaded('pcntl') in StreamSelectLoopTest.php and skipping the test if false.

The other issue I'm seeing is on HHVM 3.6.6:

1. React\Tests\EventLoop\StreamSelectLoopTest::testSignalInterruptWithStream with data set #0 ('SIGUSR1', 10)
   stream_select(): unable to select [4]: Interrupted system call (max_fd=4)

2. React\Tests\EventLoop\StreamSelectLoopTest::testSignalInterruptWithStream with data set Set autoload to false #2 ('SIGTERM', 15)
   stream_select(): unable to select [4]: Interrupted system call (max_fd=7)

[cboden]

I goofed a little bit...phpunit on my HHVM machine was running PHP5 with Debug's scream option turned on (errors not suppressed). Here is the actual error HHVM is giving me:

1. React\Tests\EventLoop\StreamSelectLoopTest::testSignalInterruptNoStream with data set #0 ('SIGUSR1', 10)
   Failed asserting that false is true.

[cebe]

Which can be fixed by checking extension_loaded('pcntl') in StreamSelectLoopTest.php and skipping the test if false.

done, ef2ced8

Need to install HHVM on my machine to test this.

[cebe]

hm... HHVM fails only on SIGUSR1 but not on the other signals...

[cebe]

Found it, the forked process needs some startup time on HHVM. Added another timer to capture the late signal.

[cboden]

Awesome! Thanks for your diligent work @cebe!

[cebe]

You're welcome, thanks for merging :)

[WyriHaximus]

Thanks for the work @cebe 👍

[mkrauser]

@cebe Yes, very nice work with the test!