New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Stream] Fix infinite loop writing to closed stream #273

Merged
merged 3 commits into from Mar 21, 2014

Conversation

Projects
None yet
4 participants
@cboden
Member

cboden commented Feb 16, 2014

refs #211

I was able to create a 100% CPU spike by not draining the buffer on the client side and then closing the connection when there was data in the write buffer on the server side.

Because feof reports true when read OR write is closed we previously tried to determine this status by meta data but that's now proved unreliable. This PR will ignore feof in the write buffer until 0 data has been sent - which either means the buffer is full (not an error) or the buffer is closed for writing.

I ran /examples/tcp-chat.php and used this script to create the 100% CPU spike:

<?php

require __DIR__ . '/../vendor/autoload.php';

$loop = React\EventLoop\Factory::create();
$dnsResolverFactory = new React\Dns\Resolver\Factory();
$dns = $dnsResolverFactory->createCached('8.8.8.8', $loop);
$connector = new React\SocketClient\Connector($loop, $dns);

$sender = null;

$connector->create('127.0.0.1', 4000)->then(function(React\Stream\Stream $stream) use (&$sender) {
    $sender = $stream;

    $much_data = str_repeat('a', 1024*1024*32);
    $stream->write($much_data);
    $stream->end();
});

$connector->create('127.0.0.1', 4000)->then(function(React\Stream\Stream $stream) use ($loop, &$sender) {
    $stream->on('data', function($data) use ($stream, &$sender) {
        echo "Pausing after receiving\n";

        $stream->pause();

        $sender->write('one more');
        $stream->close();
    });
});

$loop->run();

@cboden cboden added the streams label Feb 16, 2014

@cboden cboden referenced this pull request Feb 16, 2014

Closed

100% CPU load #137

@cboden cboden added this to the v0.3.4 milestone Feb 17, 2014

@cboden cboden added the stream label Feb 17, 2014

@mdreak

This comment has been minimized.

Show comment
Hide comment
@mdreak

mdreak Feb 20, 2014

Unfortunately, this seemed to have no effect on the 100% cpu load problem we are experiencing after several hours running the server.

mdreak commented Feb 20, 2014

Unfortunately, this seemed to have no effect on the 100% cpu load problem we are experiencing after several hours running the server.

@cboden

This comment has been minimized.

Show comment
Hide comment
@cboden

cboden Feb 20, 2014

Member

@mdreak Which EventLoop implementation are you using?

Member

cboden commented Feb 20, 2014

@mdreak Which EventLoop implementation are you using?

@RWSDev

This comment has been minimized.

Show comment
Hide comment
@RWSDev

RWSDev Feb 20, 2014

mdreak and myself (same server and implementation) were not using libevent ... I have now installed pecl-event, libevent and libevent-devel.

php -i |grep libevent
libevent2 headers version => 2.0.18-stable

I have just restarted the ratchet server and will update over the next day. It only took 12 hours to hit 100% cpu last time so we'll see.

RWSDev commented Feb 20, 2014

mdreak and myself (same server and implementation) were not using libevent ... I have now installed pecl-event, libevent and libevent-devel.

php -i |grep libevent
libevent2 headers version => 2.0.18-stable

I have just restarted the ratchet server and will update over the next day. It only took 12 hours to hit 100% cpu last time so we'll see.

@cboden

This comment has been minimized.

Show comment
Hide comment
@cboden

cboden Mar 10, 2014

Member

I was ready to merge this but the updated unit test fails on some versions of php (not the one on Travis).

Member

cboden commented Mar 10, 2014

I was ready to merge this but the updated unit test fails on some versions of php (not the one on Travis).

@cboden cboden merged commit 1590ea7 into 0.3 Mar 21, 2014

1 check passed

default The Travis CI build passed
Details
@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach Mar 21, 2014

Why closed? Will have this any solution?

hrach commented Mar 21, 2014

Why closed? Will have this any solution?

@cboden cboden deleted the 0.3-buffer-fix branch Mar 30, 2014

@cboden

This comment has been minimized.

Show comment
Hide comment
@cboden

cboden Apr 1, 2014

Member

It's closed because it was merged into all branches (0.3, 0.4, master). v0.3.4 was released that included this fix and 0.4.1 will be tagged shortly.

Member

cboden commented Apr 1, 2014

It's closed because it was merged into all branches (0.3, 0.4, master). v0.3.4 was released that included this fix and 0.4.1 will be tagged shortly.

@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach Apr 7, 2014

@cboden If I understand it correctly, it's fixed in react 0.3, however, splitsubtree react/socket is still not updated, so default my app, which requires only ratchet, is still unafected by this fix.

hrach commented Apr 7, 2014

@cboden If I understand it correctly, it's fixed in react 0.3, however, splitsubtree react/socket is still not updated, so default my app, which requires only ratchet, is still unafected by this fix.

@cboden

This comment has been minimized.

Show comment
Hide comment
@cboden

cboden Apr 8, 2014

Member

@hrach Ah, yes, you're right. I'm very close to being ready to release v0.4.1 as well. I will tag and run the sub-tree split for both versions April 12th.

Member

cboden commented Apr 8, 2014

@hrach Ah, yes, you're right. I'm very close to being ready to release v0.4.1 as well. I will tag and run the sub-tree split for both versions April 12th.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment