Changes to Buffer::write() behaviour

[bzikarsky]

The following is a proposal for a BC breaking change regarding the WritableStreamInterface::write() signature.

Currently the WritableStreamInterface does not hint on any return type. WritableStream is a void, Stream returns a boolean indicating whether the internal buffer is full (as described in the README).

This PR changes Stream, WritableStream, CompositeStream and ThroughStream so that their write-methods all return a React\Promise\PromiseInterface. Stream (or rather Buffer) resolves this promise when fwrite accepted all bytes. The other implementations all return a FulfilledPromise (instead of being a void)

Util::pipe() embraces this change by triggering pause() on the readable stream, when it encounters the full-event instead of write() returning false. CompositeStream therefore also needs to forward the full-event from the writable stream to itself.

Tests have been amended/fixed to reflect the changed behavior.

TODO:

• Discuss whether this is a sensible change 😄
• Better name for the full event? Maybe buffer-full?
• Update the documentation

[clue]

Currently the WritableStreamInterface does not hint on any return type. WritableStream is a void, Stream returns a boolean indicating whether the internal buffer is full (as described in the README).

I agree that this is odd and should be addressed. 👍

[...] write-methods all return a React\Promise\PromiseInterface

It's my understanding that most consumers of the stream API do not really care about when the buffer has been drained. In these cases, this change introduces quite a bit of overhead. Do you happen to have a particular use case for this change?

[bzikarsky]

Use-case: A CPU-intensive task is sending status updates via sockets to another system. In the first implementation I sent the status update, and scheduled task-continuation to the next (or future) tick. This resulted in the status updates never being sent. More correct would be to continue the task as soon as the status update is on the wire, hence I looked for a way to hook to this event.

But you are correct; the current implementation is probably to heavy for all those use-cases where the promised would be ignored.

Maybe we could change it that way:

• WritableStream::write(data: string): int returns the stream position for a finished write of the passed data-chunk:

  $stream = new Stream();
  $stream->write("Hello"); // 5
  $stream->write(" world!"); // 12
  

• Stream provides an additional event flush, which is triggered with the current and absolute numbers of bytes written to the stream. (not the buffer!)

Those two changes would allow for Stream to be decorated with an implementation which creates write-promises. (The implementation could be either part of react/stream or customized to the user's need in their (my 😉) package)

Alternatively this could be solved by inheritance...but I usually prefer composition over inheritance. 😄

[clue]

Use-case: A CPU-intensive task […]

I'm not sure it's likely we'll see support for blocking functions any time soon, given that even the FAQs warn against this.

continue the task as soon as the status update is on the wire […]

This should already be possible by hooking into the Buffer's full-drain event like this (untested):

$stream = new Stream($resource, $loop);
$buffer = $stream->getBuffer();
$buffer->once('full-drain', function () {
    // buffer has been flushed, now do the heavy lifting
   secondStep();
});

$response = firstStep();
$stream->write($response);

[bzikarsky]

Well, the CPU intensive task can easily split up into many parts and scheduled async. It really doesn't matter into how many parts I split, the buffer just does not seem to get flushed, if I continue with nextTick().

full-drain is my current working solution. But it now always waits for a full buffer flush, even though a a partial flush might be enough.

[clue]

Closed via #43.