fix(read stream) consume buffer multiple times #53

Pierozi · 2017-08-20T10:54:46Z

Issue related to WebSocket hoaproject/Websocket#95

We need to consume the stream until the length bytes are reach or the end of stream (if user try to get more than expected) stream_socket_recvfrom return false and fread empty value.

I've remove the read related to remoteAddress because it will be removed soon on the PR #50

Hywan

Minor changes, but I like the PR, thanks!

Hywan · 2017-08-21T07:51:13Z

Connection/Connection.php

-            $address
-        );
-        $this->_remoteAddress = !empty($address) ? $address : null;
+            if (0 === $readLength) {


I would use 0 >= $readLength in case of $readLength is lower than 0.

Hywan · 2017-08-21T07:53:01Z

Connection/Connection.php

+                $buffer = fread($this->getStream(), $readLength);
+
+                if ('' === $buffer) {
+                    $buffer = false;


Can we use another variable to stop the loop please? Something like $break. Alternatively, an infinite loop with a break would fit too.

Hywan · 2017-08-21T07:54:05Z

Travis is failing because our test suites do not work on this environment. I don't know why yet. Anyway, the PR looks good!

Should we merge #50 before?

Pierozi · 2017-08-22T09:16:05Z

We can merge this one first, I will adapt #50 when php5.x will be dropped.

coveralls · 2017-08-22T09:18:37Z

Coverage decreased (-0.5%) to 87.5% when pulling d0c10dc on Pierozi:fix/read-huge-data-from-stream into 705ec98 on hoaproject:master.

undsoft · 2017-09-07T12:24:50Z

When is this going to be merged in? Websocket server is not really usable without this.

Change made in d0c10dc

undsoft · 2017-09-07T13:56:47Z

@Pierozi Keep in mind that this single fix is not enough.
There are additional changes that need to be made to Websocket lib.
For example:

hoa/websocket/Server.php:
$buffer     = $connection->read(2048);

results in stream_socket_recvfrom waiting for message longer than the one actually sent.

Pierozi · 2017-09-07T14:02:42Z

the buffer 2048 it's only for the handshake, it should be lower than that, expect if insert very long content in headers.

undsoft · 2017-09-07T14:10:26Z

@Pierozi
In my case the handshake header is 580 characters, but read() function is expecting a length of 2048.

$connection->read(2048);

It keeps looping trying to read the string that is not there.

Pierozi · 2017-09-08T07:54:28Z

the read method should stop at length or EOF according the result of stream_socket_recvfrom or fread
do you encounter any problem?

undsoft · 2017-09-08T08:26:35Z

@Pierozi
Yeah, I don't know if this is my local config or something, but the way it works for me is this:

Execution enters read(2048).
stream_socket_recvfrom() reads the entire header (580 chars).
Loop starts another iteration and it goes to stream_socket_recvfrom().
stream_socket_recvfrom() seems to be just waiting for data. Nothing happens for at least a minute.
Execution seems to continue only when I restart the JS client. Then stream_socket_recvfrom() returns the empty string and breaks.

PHP Version 7.0.20
Darwin 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64

Hywan · 2017-09-08T09:03:44Z

I wait your issue to be fixed to merge this PR.

undsoft · 2017-09-08T10:02:05Z

@Pierozi
I can hack around this by adding another param:

public function read($length, $flags = 0, $keepReading = true)
    {
        if (null === $this->getStream()) {
            throw new Socket\Exception(
                'Cannot read because socket is not established, ' .
                'i.e. not connected.',
                1
            );
        }

        if (0 > $length) {
            throw new Socket\Exception(
                'Length must be greater than 0, given %d.',
                2,
                $length
            );
        }

        $out = '';

       do {
            $readLength = $length - strlen($out);
            if (0 >= $readLength) {
                break;
            }
            if (true === $this->isEncrypted()) {
                $buffer = fread($this->getStream(), $readLength);
            } else {
                $buffer = stream_socket_recvfrom($this->getStream(), $readLength, $flags);
            }
            if ('' !== $buffer && is_string($buffer)) {
                $out .= $buffer;
            } else {
                break;
            }
        }
        while ($keepReading);

        return $out;
    }

and then changing socket/Server and socket/Client to use it:

$buffer     = $connection->read(2048, 0, false);

but there must be a better solution.

…nsumer

coveralls · 2017-10-22T13:10:00Z

Coverage decreased (-0.7%) to 87.365% when pulling f89af35 on Pierozi:fix/read-huge-data-from-stream into 705ec98 on hoaproject:master.

Pierozi · 2017-10-22T13:18:44Z

So, I've taken some time to deep dive into this issue.

My original patch works great when Stream is not in blocking mode, and solve the issue related to read large stream.

But with Hoa\Websock we open blocked Stream, and when the handshake is less than 2048 bytes (value use in WebSocket), the \Hoa\Connection\read() method will continue to consume stream and wait until new data arrived.

We cannot assume to stop if the first result is less than the number of bytes we seeking because we are not sure of the size of the block returned. In my test, even when stream_set_chunk_size defined to 1024 bytes, and playing with a large stream (6MB), some of the results were less than my chunk. Because it depends that how fast the client will consume the stream while the server pushes into it at the same time.

\Hoa\Connection\read() result expected is the number of bytes specified in the argument.
If the stream is large, we cannot afford this number of bytes without using multiples times consume methods.
If the stream is in blocking mode, we are facing potential blocking mode.

Backward Compatibility

By introducing $keepReading at true by default we facing potential BC break for peoples that may use blocking stream but in same time resolve issue fo who read large stream.
The impact on Hoa\WebSocket is that we have to set keepReading false on doHandshake() method.

keepReading false default

Anyone who wants to consume large stream will need to set the boolean to true
The impact on Hoa\WebSocket is on read() method used in Protocols class, mainly under readFrame() to specify keepReading at true in order to make large stream works.

I vote for BC break because read length should be the natural behavior of the read method.

@undsoft @Hywan what's your thought?

Pierozi · 2017-11-01T08:14:57Z

@Hywan ping there

Hywan · 2017-11-06T10:44:43Z

Connection/Connection.php

@@ -681,21 +683,31 @@ public function read($length, $flags = 0)
            );
        }

-        if (true === $this->isEncrypted()) {
-            return fread($this->getStream(), $length);
+        if (is_int($chunk)) {


- if (is_int($chunk)) + if (is_int($chunk) && 0 < $chunk))

to ensure the chunk is greater than zero. Could it be greater than or equal to zero?

You right! never trust the userland!

Hywan · 2017-11-06T10:45:36Z

Connection/Connection.php

-        if (true === $this->isEncrypted()) {
-            return fread($this->getStream(), $length);
+        if (is_int($chunk)) {
+            stream_set_chunk_size($this->getStream(), $chunk);


Should we collect the previous chunk size (returned by stream_set_chunk_size) and restore it just after the call? I'm not sure how it acts with concurrent accesses…

Hywan · 2017-11-06T10:47:48Z

@Pierozi Can we guess the default value for $keepReading based on the blocking mode?

Pierozi · 2017-11-06T15:29:11Z

I need to check related to Hoa\Stream but I guess it's a good option.

coveralls · 2017-11-20T22:09:53Z

Coverage decreased (-0.9%) to 87.12% when pulling 2aa5cb3 on Pierozi:fix/read-huge-data-from-stream into 705ec98 on hoaproject:master.

coveralls · 2017-11-20T22:11:32Z

Coverage decreased (-0.9%) to 87.12% when pulling 2aa5cb3 on Pierozi:fix/read-huge-data-from-stream into 705ec98 on hoaproject:master.

Pierozi · 2017-11-20T22:16:07Z

@Hywan detection using getStreamMetaData.

First thought was to use Hoa\Option for $keepReading; because with PHP 7.1 strict, we will not able to type in boolean due to nulled value.
Unfortunately, Interface Stream\IStream\In constraint us to set a default value. (or adapt interface)
So, I'm trapped with the null.

fix(read stream) consume buffer multiple times

a709466

Pierozi self-assigned this Aug 20, 2017

Pierozi requested a review from Hywan August 20, 2017 10:54

Hywan previously requested changes Aug 21, 2017

View reviewed changes

Hywan added bug difficulty: hard in progress labels Aug 21, 2017

Hywan assigned Hywan and unassigned Pierozi Aug 21, 2017

fix(read stream) keep it simple

d0c10dc

Pierozi requested a review from Hywan September 7, 2017 13:01

fix(read stream) parameter keepReading and chunk to control stream co…

f89af35

…nsumer

ghost assigned Pierozi Oct 22, 2017

Hywan reviewed Nov 6, 2017

View reviewed changes

Pierozi added 2 commits November 20, 2017 23:06

fix(read stream) dynamic default value

41cb8bb

CS

2aa5cb3

Pierozi mentioned this pull request Mar 18, 2019

stream_socket_recvfrom returning false #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(read stream) consume buffer multiple times #53

fix(read stream) consume buffer multiple times #53

Pierozi commented Aug 20, 2017

Hywan left a comment

Hywan Aug 21, 2017

Hywan Aug 21, 2017

Hywan commented Aug 21, 2017

Pierozi commented Aug 22, 2017

coveralls commented Aug 22, 2017 •

edited

Loading

undsoft commented Sep 7, 2017

undsoft commented Sep 7, 2017

Pierozi commented Sep 7, 2017

undsoft commented Sep 7, 2017

Pierozi commented Sep 8, 2017

undsoft commented Sep 8, 2017

Hywan commented Sep 8, 2017

undsoft commented Sep 8, 2017

coveralls commented Oct 22, 2017 •

edited

Loading

Pierozi commented Oct 22, 2017

Pierozi commented Nov 1, 2017

Hywan Nov 6, 2017

Pierozi Nov 6, 2017

Hywan Nov 6, 2017

Hywan commented Nov 6, 2017

Pierozi commented Nov 6, 2017

coveralls commented Nov 20, 2017 •

edited

Loading

coveralls commented Nov 20, 2017 •

edited

Loading

Pierozi commented Nov 20, 2017

fix(read stream) consume buffer multiple times #53

Are you sure you want to change the base?

fix(read stream) consume buffer multiple times #53

Conversation

Pierozi commented Aug 20, 2017

Hywan left a comment

Choose a reason for hiding this comment

Hywan Aug 21, 2017

Choose a reason for hiding this comment

Hywan Aug 21, 2017

Choose a reason for hiding this comment

Hywan commented Aug 21, 2017

Pierozi commented Aug 22, 2017

coveralls commented Aug 22, 2017 • edited Loading

undsoft commented Sep 7, 2017

undsoft commented Sep 7, 2017

Pierozi commented Sep 7, 2017

undsoft commented Sep 7, 2017

Pierozi commented Sep 8, 2017

undsoft commented Sep 8, 2017

Hywan commented Sep 8, 2017

undsoft commented Sep 8, 2017

coveralls commented Oct 22, 2017 • edited Loading

Pierozi commented Oct 22, 2017

Backward Compatibility

keepReading false default

Pierozi commented Nov 1, 2017

Hywan Nov 6, 2017

Choose a reason for hiding this comment

Pierozi Nov 6, 2017

Choose a reason for hiding this comment

Hywan Nov 6, 2017

Choose a reason for hiding this comment

Hywan commented Nov 6, 2017

Pierozi commented Nov 6, 2017

coveralls commented Nov 20, 2017 • edited Loading

coveralls commented Nov 20, 2017 • edited Loading

Pierozi commented Nov 20, 2017

coveralls commented Aug 22, 2017 •

edited

Loading

coveralls commented Oct 22, 2017 •

edited

Loading

coveralls commented Nov 20, 2017 •

edited

Loading

coveralls commented Nov 20, 2017 •

edited

Loading