Skip to content

Conversation

@arnaud-lb
Copy link
Contributor

When write() is called in the event handler of a drain event, Buffer can write things twice.

Example:

$buffer = new Buffer();
$buffer->once('drain', function ($buffer) {
    $buffer->write("bar\n");
});

$buffer->write("foo\n");

This write "foo\nfoo\nbar\n" instead of just "foo\nbar\n".

This doesn't happen if $buffer->write("bar\n"); is called on the next tick (which explains why it usually works).

This happens in actual code, e.g. when using Util::pipe() on a Readable which immediately emits a 'data' event when resume() is called.

This PR fixes this by updating the internal buffer before emitting the drain event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow, are these assignments required only in the testing environment to prevent actual writes? Some background info and/or documentation would be much appreciated! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I duplicated what testDrain() was doing; it does this to simulate a stream that's not immediately available for writes.

@clue
Copy link
Member

clue commented Jun 12, 2015

Awesome, thanks for spotting and filing this PR! 👍

See my above remark about the test case, otherwise this LGTM 👍

@clue
Copy link
Member

clue commented Sep 6, 2015

Thanks for clearing this up @arnaud-lb! LGTM 👍

@cboden cboden added this to the v0.4.3 milestone Sep 9, 2015
@clue
Copy link
Member

clue commented Sep 18, 2015

This happens in actual code, e.g. when using Util::pipe() on a Readable which immediately emits a 'data' event when resume() is called.

IMO this is indeed a major issue.

👍 for getting this in sooner rather than later

Ping @cboden, @WyriHaximus

@WyriHaximus
Copy link
Member

LGTM 👍

cboden added a commit that referenced this pull request Oct 1, 2015
Fixed double-writes when write() is called during 'drain' event
@cboden cboden merged commit 5d89f2a into reactphp:master Oct 1, 2015
@cboden
Copy link
Member

cboden commented Oct 1, 2015

Thanks @arnaud-lb! Great work!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants