Issue with suppressed errors and empty error_get_last

[brstgt]

https://github.com/amphp/byte-stream/blob/6d6c89f58c213e600e2ab5e3b1678fb61333eeb7/lib/ResourceOutputStream.php#L70

I currently debugged an issue in IpcLogger of aerys. It hang with 100% CPU as the socket was not detected as dead. In my setup @fwrite does NOT populate error_get_last for some reason. Furthermore it also was not populated if the registered error handler returned 'true'.

The only really reliable solution (in IpcLogger) was like that:

        $eh = set_error_handler([$this, 'onDeadIpcSock']);
        $bytes = fwrite($this->ipcSock, $this->writeBuffer);
        set_error_handler($eh);

If it affects that code then I am pretty sure it also affects the byte-stream code, right? Not sure if this is a bug in PHP because in other places, the @ / error_reporting(0) DID populate the error_get_last

Any other idea to catch fwrite errors in a better way?

[kelunik]

Which error message do you get with that approach?

[brstgt]

Found it. Actually it was an unwanted behaviour in my error handler but this still introduces a possible bug as that code is functionally depending on the return code of a registered error handler.

Fact: If the registered error handler returns "true" on a suppressed error then ResourceOutputStream will not detect broken pipes. The socket will be writeable forever, never be cancelled and a loop could hang and run with 100% CPU

[brstgt]

@kelunik instead of error_get_last the error is simply passed to your temp error handler. You can then handle the error in the same way as you do already

[kelunik]

I'm asking because we see behavior with 100% CPU currently if you run something with cluster and add fwrite(STDOUT, "foo"); in the worker, which breaks the IPC channel and makes the parent exit, but the other children will still be alive and 100% CPU then.

[brstgt]

Yep, that's probably the same or related issue. Currently the IpcLogger in aerys isn't checking for error_get_last at all - that's why the shutting down WorkerProcess hangs forever if there's sth to log at shutdown. The parent closes the IPC sock earlier and the child always gets EPIPE.

For Ipc a better approach was that the parent doesn't close the socket at all. The child should close it when everything is shut down correctly. The parent should just watch for the closed socket to clean everything.

[kelunik]

@amphp/windows-tester Could any of you please run the current master test suite on Windows and report the results?

[pcrov]

There were 2 failures:

1) Amp\ByteStream\Test\ResourceOutputStreamTest::testBrokenPipe Failed asserting that exception of type "Amp\ByteStream\StreamException" is thrown.

2) Amp\ByteStream\Test\ZlibInputStreamTest::testRead Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@

Warning: Strings contain different line endings!

-'foobar content +'foobar content

[trowski]

@pcrov Can you try again with the commit I just pushed?

[pcrov]

There was 1 failure:

1) Amp\ByteStream\Test\ZlibInputStreamTest::testRead Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@

Warning: Strings contain different line endings!

-'foobar content +'foobar content

[trowski]

Thanks @pcrov, that test probably needs to be fixed on Windows, but that has nothing to do with this issue, so hopefully this has been fixed. I'll wait for @kelunik to confirm before tagging.

[kelunik]

@pcrov Does it work now?

[pcrov]

PHPUnit 6.5.7 by Sebastian Bergmann and contributors.

................................................................. 65 / 70 ( 92%)
.....                                                             70 / 70 (100%)

Time: 1.13 seconds, Memory: 8.00MB

OK (70 tests, 98 assertions)

[kelunik]

@pcrov Thanks for confirming!