Using predis-async from within React Http Server

[Andrewpk]

CPU spikes to 100% regardless of the event loop backend I use and feed to Predis\Async\Connection\StreamConnection.
Can't seem to pinpoint the issue, but it seems to have creeped up when I moved to my new server (php 5.4, new linux kernel - lots of variables in the equation).

CPU only spikes after a command is issued to redis.
I'm going to keep trying to track down exactly what is causing this in my free time, I just wanted to let you know.

To recreate:

• Create a React\Http\Server using the example in the React repo.
• Instantiate a redis connection using predis-async manually and feed it the same event loop instance you used to create your React\Http\Server.
• in http->on('request') - issue a simple command to $redis via your manual connection using predis-async. The http response doesn't need to contain any redis related results, but be sure to respond/end the http request.
• start the react HTTP server and issue a GET request via your browser to the running server. Your cpu should now be at 100% on one core.

[bixuehujin]

It seems because of the underlying eventloop implementation (StreamSelect, LibEventLoop) all using level triggered I/O. Once a stream is writable, the wriable event will be triggered again and again, that it why CPU spikes to 100%.

After digging into the source of predis-ansync, i hacked the StreamConnection of predis-ansync, and it seems ok now. see bixuehujin@63aaeaa

[tomsseisums]

Since I had stumbled upon the same issue, I just started researching it.

@bixuehujin, applied your fix, it fixed it. Plus, it seems logical that it should fix it.

[nrk]

Sorry for the late response @Andrewpk, I have yet to check the changes in @bixuehujin's PR (btw thanks in advance!) but I can confirm this bug when using Predis\Async v0.2.0 while everything seems to be working just fine with v0.1.0.

Now the only change between the two releases is a dependency upgrade of react/event-loop (from v0.2 to v0.3) that involved slight changes in how we handle connection timeouts due to a different abstraction used for timers. I'm not sure right now how this could have affected Predis\Async, but it seems to be a fact that something broke in the process.

I think I'll be able to sort it out by tomorrow applying @bixuehujin's PR if I can't find what exactly happened.

[bixuehujin]

Looked at the ChangeLog of react/event-loop, a BC break was made in version 0.3.0

BC break: [EventLoop] Remove check on return value from stream callbacks

That is, we must remove write stream from event loop manually once there is no more data to write.

[nrk]

@bixuehujin ... and just to add insult to injury, I was the one that committed that change to react/event-loop! :-)

Now seriously, it completely slipped my review when I modified Predis\Async as I was pretty certain (but turns out I was wrong) I didn't use return values from callbacks to remove streams from the loop. But then to fix the bug wouldn't it be enough to change this part into:

$socket = $this->getResource();

if ($this->buffer->isEmpty()) {
    $this->loop->removeWriteStream($socket);
    return;
}

I can't test right now as I won't have access to a dev box until tomorrow afternoon CEST.

[bixuehujin]

After a twice think and review of my PR, Its not wise to rely StreamConnection::commands->isEmpty() to detect whether has more data to write. Because this will lead a lot of empty event loop before data received from redis.

Actually, i always want to avoid to add/remove stream to/from loop frequently, but it seems not easy to achieve in a level triggered loop.

[nrk]

Pushed a fix but have yet to tag a new release, now everything seems OK and you can actually try that by using "predis/predis-async": "dev-master". If anyone can confirm that the issue is fixed for them, I'll prepare a new release ASAP.

@bixuehujin yeah but we can't really do much considered how the event notifications are triggered by the underlying loop.

[tomsseisums]

Just updated my environment, discarded @bixuehujin temp-fix, launched script - works flawlessly.

thumbs up

[nrk]

Alright pushed v0.2.1 just a few moments ago.

Thanks everyone :-)