Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use default stream data handler #33

Closed
wants to merge 2 commits into from

Conversation

dercoder
Copy link

@dercoder dercoder commented May 4, 2015

This SecureStream data handler uses "stream_get_contents" which will handle data only when connection has been closed. Use "fread" instead which will be used by default in "Stream" Class

@cboden
Copy link
Contributor

cboden commented May 4, 2015

See #14, #24, #26

@dercoder
Copy link
Author

dercoder commented May 4, 2015

I have checked this issues but none of them will resolve this "stream_get_contents" issue

@cboden
Copy link
Contributor

cboden commented May 4, 2015

Can you post some code to reproduce the problem you have? In all our tests stream_get_contents has fixed SSL issues we've seen. From the PHP website:

Reads remainder of a stream into a string...operates on an already open stream resource and returns the remaining contents in a string

if ($stream = fopen('http://www.example.com', 'r')) {
    // print all the page starting at the offset 10
    echo stream_get_contents($stream, -1, 10);

    fclose($stream);
}

The documentation and examples show stream_get_contents operate on an opened resource stream.

@dercoder
Copy link
Author

dercoder commented May 4, 2015

This works fine with webserver but i have to connect to socket server which sends data all the time. So the handler will not work until connection will be closed

@cboden
Copy link
Contributor

cboden commented May 4, 2015

Please post code to reproduce this issue.

I have personally tested this code against an HTTPS server and an SSL IRC server; which does not close the connection after receiving data via stream_get_contents. The function drains the buffer of data, it does not fetch data after the connection has closed.

@dercoder
Copy link
Author

dercoder commented May 4, 2015

This is my example code:

$host = 'live.example.com';
$port = 1234;
$username = 'user1';
$password = '123456';

$loop = \React\EventLoop\Factory::create();
$dnsResolver = new \React\Dns\Resolver\Factory();
$dns = $dnsResolver->createCached('8.8.8.8', $loop);

$connector = new \React\SocketClient\Connector($loop, $dns);
$secureConnector = new \React\SocketClient\SecureConnector($connector, $loop);

$secureConnector->create($host, $port)->then(function (\React\Stream\Stream $stream) use ($username, $password) {

    $this->log('CONNECTED');

    $stream->on('data', function ($data) {
        $this->log('DATA -> ' . $data);
    });

    $stream->on('error', function ($error) {
        $this->log('ERROR');
    });

    $stream->on('close', function () {
        $this->log('CLOSE');
    });

    $stream->write('<login><credential><loginname value="' . $username . '"/><password value="' . $password . '"/></credential></login>' . "\r\n\r\n");

});

$loop->run();

Result with "stream_get_contents":

23:30:50: CONNECTED
23:31:51: DATA -> <login result="valid">
<user id="XXXX"/>
</login>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>

<ct/>


23:31:51: CLOSE

Result with "fread":

23:34:00: CONNECTED
23:34:00: DATA -> <login result="valid">
<user id="XXXX"/>
</login>


23:34:01: DATA -> <
23:34:04: DATA -> ct/>


23:34:04: DATA -> <
23:34:07: DATA -> ct/>


23:34:07: DATA -> <
23:34:10: DATA -> ct/>


23:34:10: DATA -> <
23:34:13: DATA -> ct/>


23:34:13: DATA -> <
23:34:16: DATA -> ct/>


23:34:16: DATA -> <
23:34:19: DATA -> ct/>


23:34:19: DATA -> <
23:34:22: DATA -> ct/>


23:34:22: DATA -> <
23:34:25: DATA -> ct/>


23:34:25: DATA -> <
23:34:28: DATA -> ct/>


23:34:28: DATA -> <
23:34:31: DATA -> ct/>


23:34:31: DATA -> <
23:34:34: DATA -> ct/>


23:34:34: DATA -> <
23:34:37: DATA -> ct/>


23:34:37: DATA -> <
23:34:40: DATA -> ct/>


23:34:40: DATA -> <
23:34:43: DATA -> ct/>


23:34:43: DATA -> <
23:34:46: DATA -> ct/>


23:34:46: DATA -> <
23:34:49: DATA -> ct/>


23:34:49: DATA -> <
23:34:52: DATA -> ct/>


23:34:52: DATA -> <
23:34:55: DATA -> ct/>


23:34:55: DATA -> <
23:34:58: DATA -> ct/>


23:34:58: DATA -> <
23:35:01: DATA -> ct/>


23:35:01: DATA -> <
23:35:01: DATA -> ct/>


23:35:01: CLOSE

One thing I have noticed is that SSL connection read is really slow.

23:34:58: DATA -> <
23:35:01: DATA -> ct/>

It took 3 seconds to read this XML string. Without SSL its working fine and fast.

@dercoder
Copy link
Author

dercoder commented May 5, 2015

I have found a solution to avoid this blocking EOF check

@elazar
Copy link

elazar commented May 13, 2015

Using this version: PHP 5.5.9-1ubuntu4.5 (cli) (built: Oct 29 2014 11:59:10)

Can't say much except that I'm able to connect to the Freenode IRC network using SSL with @dercoder's patch and I'm not noticing any blocking or other abnormal behavior.

@dercoder
Copy link
Author

Can you show me your code so i can test it? I am using PHP 5.6.8-1

@elazar
Copy link

elazar commented May 13, 2015

@dercoder
Copy link
Author

Please check your Gist. Does your code work with SSL on PHP 5.5.9?

@elazar
Copy link

elazar commented May 13, 2015

@dercoder It does work with your patch. I haven't tested it without the patch.

@dercoder
Copy link
Author

Can you please try it without Patch + SSL?

@elazar
Copy link

elazar commented May 13, 2015

Hm... it seems to work the same regardless of whether the patch is applied or not, from my point of view.

@dercoder
Copy link
Author

So it seems to be a PHP 5.6 issue

@elazar
Copy link

elazar commented May 13, 2015

I don't have an environment readily available to confirm, but I should later on tonight (I'm UTC-5 at the moment).

@cboden
Copy link
Contributor

cboden commented May 13, 2015

It might be a 5.6.8 specific issue. 68853 and 65137 were addressed in that release.

Are you able to test on a 5.6 version lower than 5.6.8?

Also #34 was merged this morning (related to the 5.6.8 release). master branch might have inadvertently fixed this for you already.

@cboden
Copy link
Contributor

cboden commented May 13, 2015

I wonder if this is related to reactphp/reactphp#273

Because feof reports true when read OR write is closed we previously tried to determine this status by meta data but that's now proved unreliable

@cboden
Copy link
Contributor

cboden commented May 14, 2015

I have PHP 5.5.20 and 5.4.6 locally to test on. 5.5.20 seems to have the same SSL fix 5.6.8 has. On 5.4.6 I can see the SSL non-draining buffer bug related to #14, #24, and #26. To re-enable that bug I have completely disabled the wrapSecure in StreamEntryption which replaced fread with stream_get_contents. AFAIK to fix that bug I have seen 3 ways:

  • Use stream_get_contents instead of fread
  • Upgrad to PHP 5.6.8 and use fread
  • Use stream_get_meta_data()['eof'] instead of feof (what this PR is doing)
  • Call fread again (this is not an option)

I'm not sure how to specifically trigger this bug though. I use a phergie script and have seen at the React level of it where the buffer drain isn't happening. I'm still waiting on @dercoder to test the master branch, where >=5.6.8 ignores the secure wrap so always uses fread + feof. However, @edhelas has reported the issue not fixed as well as 68853. If one of you guys could leave a simple script that I could copy/paste to show me the bug that'd be much appreciated (just a client script connecting to some server somewhere).

My concern with this PR is the following from the php.net documentation on stream_get_meta_data()['eof']:

Note that for socket streams this member can be TRUE even when unread_bytes is non-zero. To determine if there is more data to be read, use feof() instead of reading this item

I don't remember the details but I recall @DaveRandom looking into the source code of both of these and feof() would trigger actions whereas the metadata would just read a variable already set. I fear this could open up edge case bugs.

@dercoder
Copy link
Author

I have tested the master branch with my script and phergie script. Its working fine now on PHP 5.6.8.

@elazar
Copy link

elazar commented Jul 15, 2015

@dercoder @cboden Should we close this out unmerged then?

@dercoder
Copy link
Author

Yes, i think you can close it

@clue
Copy link
Contributor

clue commented Sep 7, 2015

Yes, i think you can close it

Thanks for the confirmation!

@clue clue closed this Sep 7, 2015
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.

None yet

4 participants