Optional maximum length argument for the buffer function by WyriHaximus · Pull Request #3 · reactphp/promise-stream

WyriHaximus · 2017-10-10T20:23:37Z

As discussed on hangouts added an optional maximum length argument to the buffer function.

clue

Nice progress, would love to get this feature in!

clue · 2017-10-10T20:45:57Z

README.md

-a `Promise` which resolves with the stream data buffer.
+The `buffer(ReadableStreamInterface $stream, int $maxLength = 0)` function can be used to create
+a `Promise` which resolves with the stream data buffer. With an optional maximum length argument 
+which defaults to no limit.


IMO 0 is a valid limit to ensure no data is sent. I think a default null parameter to mean "no limit" makes more sense here 👍

Changed it, but using a limit of 0 is kinda pointless 🤐

I agree that there aren't many use cases for this, but IMO it makes more sense as a extreme value instead of "no value" which would be closer to the other end of the range :-)

clue · 2017-10-10T20:47:12Z

README.md

+    var_dump(json_decode($contents));
+}, function ($error) {
+    // Reaching here when the stream buffer goes above the max size.
+    // (In this example that is 1024 bytes.)


Note that the promise will also be rejected when a stream error is emitted or when the promise is cancelled.

clue · 2017-10-10T20:48:17Z

src/functions.php


-    $promise = new Promise\Promise(function ($resolve, $reject) use ($stream, &$buffer) {
+    $promise = new Promise\Promise(function ($resolve, $reject) use ($stream, $deferred, &$buffer) {
+        $deferred->promise()->done($resolve, $reject);


This requires Promise v2 API.

clue · 2017-10-10T20:48:34Z

src/functions.php

        $buffer .= $data;
+        if ($maxLength > 0 && strlen($buffer) > $maxLength) {
+            $deferred->reject(new \RuntimeException('Buffer exceeded maximum length'));
+        }


Should probably also stop buffering in the background?

Actually when the promise is rejected it is automatically removing the buffer listener

WyriHaximus · 2017-10-10T21:16:45Z

@clue updated the PR, will squash it once all feedback has been worked in to it and both of you approved the PR 👍

jsor · 2017-10-11T07:36:24Z

src/functions.php

+        if ($maxLength !== null && strlen($buffer) > (int)$maxLength) {
+            $deferred->reject(new \RuntimeException('Buffer exceeded maximum length'));
+        }
    };


Probably micro-optimization, but what about incremental buffer size counting?

$buffer = ''; $bufferLength = 0; $bufferer = function ($data) use (&$buffer, &$bufferLength, &$bufferer, $deferred, $maxLength) { $buffer .= $data; if (null === $maxLength) { return; } $bufferLength += strlen($data); if ($bufferLength > $maxLength) { $deferred->reject(new \RuntimeException('Buffer exceeded maximum length')); } };

Talking about optimizations, strlen() is actually O(1), so not much would be gained :-) As an alternative, you can use isset($buffer[$maxLength]) to avoid the expensive function call 👍

WyriHaximus · 2017-10-11T11:48:40Z

@jsor @clue applied your suggestions and squashed the commits 👍

kelunik · 2017-10-11T11:50:55Z

src/functions.php

+    $bufferer = function ($data) use (&$buffer, &$bufferer, $stream, $deferred, $maxLength) {
        $buffer .= $data;
+        if ($maxLength !== null && isset($buffer[$maxLength])) {
+            $deferred->reject(new \RuntimeException('Buffer exceeded maximum length'));


This doesn't unregister the data callback, so it might result in a double resolution of the deferred, no?

There is no double resolution in react/promise, first come, first serve. Subsequent resolutions are ignored.

There is room for improvement for this function in general, but that's something for a follow-up PR.

What @jsor says, but also when rejecting there the error passes through https://github.com/WyriHaximus-labs/promise-stream/blob/7f505f3915e331b90b6e3a6040a33d4104243e15/src/functions.php#L49-L55 which does remove the listener

clue · 2017-10-13T08:04:36Z

src/functions.php

 * Creates a `Promise` which resolves with the stream data buffer
 *
 * @param ReadableStreamInterface $stream
+ * @param int $maxLength


@param int|null $maxLength maximum number of bytes to buffer or null=unlimited? See also signature in README.

clue · 2017-10-13T08:05:52Z

src/functions.php

+    $bufferer = function ($data) use (&$buffer, $deferred, $maxLength) {
        $buffer .= $data;
+        if ($maxLength !== null && isset($buffer[$maxLength])) {
+            $deferred->reject(new \RuntimeException('Buffer exceeded maximum length'));


I would suggest using an OverflowException here and explicitly document this in the README. This way, the user can easily discern normal stream errors from buffer overflow errors 👍

Same as my comment here reactphp/http#232 (comment), OverflowException would be nice.

clue · 2017-10-13T08:06:49Z

tests/BufferTest.php

+
+    public function testMaximumSize()
+    {
+        $this->setExpectedException('\RuntimeException', 'Buffer exceeded maximum length');


Should be moved down to the Block\await()?

WyriHaximus · 2017-10-13T10:14:46Z

@clue @andig Updated per your suggestions 👍 /cc @jsor

andig · 2017-10-15T09:39:17Z

Can this PR be readied for merge + release as it blocks HTTP Server?

clue · 2017-10-15T10:26:46Z

src/functions.php

+    $bufferer = function ($data) use (&$buffer, $deferred, $maxLength) {
        $buffer .= $data;
+        if ($maxLength !== null && isset($buffer[$maxLength])) {
+            $deferred->reject(new \OverflowException('Buffer exceeded maximum length'));


LGTM, but should be added to documentation 👍

Will do 👍

clue

Nice, squashed changes, now let's get this in!

WyriHaximus · 2017-10-17T12:08:43Z

@clue awesome! Thanks for the squashing 👍

WyriHaximus added easy pick help wanted new feature labels Oct 10, 2017

WyriHaximus requested review from clue and jsor October 10, 2017 20:23

WyriHaximus added this to the v0.1.2 milestone Oct 10, 2017

clue requested changes Oct 10, 2017

View reviewed changes

jsor reviewed Oct 11, 2017

View reviewed changes

WyriHaximus force-pushed the buffer-max-size branch from 010e235 to 7f505f3 Compare October 11, 2017 11:39

kelunik reviewed Oct 11, 2017

View reviewed changes

WyriHaximus force-pushed the buffer-max-size branch from 7f505f3 to 0ab3812 Compare October 11, 2017 17:37

WyriHaximus mentioned this pull request Oct 12, 2017

RequestBodyBufferMiddleware: 411 No Content Length for GET requests? reactphp/http#232

Closed

clue reviewed Oct 13, 2017

View reviewed changes

clue removed the easy pick label Oct 13, 2017

WyriHaximus force-pushed the buffer-max-size branch from 8784edc to 5835e5b Compare October 13, 2017 10:12

WyriHaximus force-pushed the buffer-max-size branch from 5835e5b to 2744beb Compare October 13, 2017 14:25

andig mentioned this pull request Oct 13, 2017

Support buffering requests with unknown length (Transfer-Encoding: chunked) in RequestBodyBufferMiddleware reactphp/http#235

Merged

clue reviewed Oct 15, 2017

View reviewed changes

Optional maximum length argument for the buffer function

39ca554

clue force-pushed the buffer-max-size branch from 1748831 to 39ca554 Compare October 17, 2017 12:00

clue approved these changes Oct 17, 2017

View reviewed changes

jsor approved these changes Oct 17, 2017

View reviewed changes

jsor merged commit 6a88554 into reactphp:master Oct 17, 2017

Uh oh!

Conversation

WyriHaximus commented Oct 10, 2017

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Oct 13, 2017

Uh oh!

andig commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WyriHaximus commented Oct 11, 2017 •

edited

Loading