Emit error event and close Stream when writing to stream resource fails

[bohdanly]

Hi.
I have Ratchet working 24/7 and sometimes it starting to use 100% of CPU. In this case I need to restart it and this helps. But it happend very often, so I debug it and found that foef($this->stream) do not return true when client disconnects while event loop tick is not ended.
Same with is_resource($this->stream) it returns true.

I'm using:

• PHP 5.5.13 / Win8 x64
• PHP 5.4.10 / Win7 x64
• react/event-loop 0.4.1
• react/socket 0.4.2
• react/stream 0.4.2
• cboden/ratchet 0.3.3

In this case after

$sent = fwrite($this->stream, $this->data);

$this->lastError['message'] contains an error:

There is no way to catch such behavior using best practices, so I decide to check $this->lastError['message'] on 'errno=10054'.
Maybe it will be more efficient if we will check $this->lastError['number'] on any error > 0. But to prevent just on this case I decide to check only errno=10054 in message.

I know this is not good resolution but it works.

[cboden]

Excellent troubleshooting @lysenkobv! I agree with you it doesn't look like a good resolution but PHP socket error handling probably doesn't allow something more elegant.

Are you able to provide a unit test proving and fixing this or steps to reproduce so we can see/test this manually?

[WyriHaximus]

Impressive find 👍 , it isn't not neat but it's the best we can get, LGTM 👍

[bohdanly]

I wrote a test and found that BufferTest::testWritingToClosedStream() have the same logic I wrote.
I decide to not duplicate it. But it is required to make some changes in the code.

When $sent = fwrite($this->stream, $this->data); executes we need to check errors.
Better solution is to check errno. set_error_handler always handle error if one occurred.

if ($this->lastError['number'] > 0) {

In this case, that part of code is not needed anymore:

if (0 === $sent && feof($this->stream)) {
     $this->emit('error', array(new \RuntimeException('Tried to write to closed stream.'), $this));
     return;
}

...because in test testWritingToClosedStream() this type of error is catched by set_error_handler().
Error message contains fwrite(): send of 3 bytes failed with errno=32 Broken pipe, errno=8.

To be sure I replaced part of code I mentioned above by checking $sent === false

if ($sent === false) {
    $this->emit('error', array(new \RuntimeException('Send failed'), $this));
    return;
}

In conclusion error checking looks like that:

if ($this->lastError['number'] > 0) {
    $this->emit('error', array(
        new \ErrorException(
            $this->lastError['message'],
            0,
            $this->lastError['number'],
            $this->lastError['file'],
            $this->lastError['line']
        ),
        $this
    ));

    return;
}

if ($sent === false) {
    $this->emit('error', array(new \RuntimeException('Send failed'), $this));
    return;
}

and testWritingToClosedStream asserion looks like that:

$this->assertSame('fwrite(): send of 3 bytes failed with errno=32 Broken pipe', $error->getMessage());

All 66 tests have executed successfully.

[bohdanly]

Is anybody here? 😄

[cboden]

👍 Again, just awesome work on this @lysenkobv! I'm very weary about changing the buffer code because of stupid strange edge cases that still haunt me to this day, but after much deliberation and testing am comfortable with this change. :-)

[clue]

Unfortunately this change is known to cause issues with running the Autobahn testsuite against Ratchet. Thanks to @cboden for digging into this!

I'm currently looking into providing a test case so that this is properly covered in the Stream component.

[cboden]

It ended up being line 90. The error I'm receiving is:

fwrite(): send of 8192 bytes failed with errno=11 Resource temporarily unavailable

On attempting to send a large amount of data (256k) to fwrite. The error simply states the buffer is full and to try again later, however the updated code now halts on all errors.

[clue]

This will be fixed via #52.