Skip to content

Work around PHP bug of liveness checking#643

Merged
michael-grunder merged 1 commit intophpredis:developfrom
meitu:develop
Nov 18, 2015
Merged

Work around PHP bug of liveness checking#643
michael-grunder merged 1 commit intophpredis:developfrom
meitu:develop

Conversation

@shafreeck
Copy link
Contributor

@shafreeck shafreeck commented Aug 5, 2015

I think this patch may fix #70 .

There are two main reason to cause "read error on connecion".

  1. PHP has a bug of checking liveness of a connection. It may
    have a false positive judgement when a connection is indeed closed.
  2. phpredis does not check if the redis command is sent successfully or not.

You can reproduce it easily

  1. Modify library.c and set errno to EWOULDBLOCK before any php_stream_eof. This will trigger the PHP bug.
  2. Create test.php
<?php
$rds = new Redis();
try {
    $ret = $rds->pconnect("127.0.0.1", 6379);
    if ($ret == false) {
        echo "Connect return false";
        exit;
    }

    echo "Press any key to continue ...";
    fgetc(STDIN);
    var_dump($rds->get("hellophpredis"));
} catch (Exception $e) {
    var_dump ($e);
}
?>
  1. Run php test.php
  2. Login redis and client kill the test connection
  3. Return back to test.php and press any key. Now you get the exception.

@michael-grunder
Copy link
Member

michael-grunder commented Aug 5, 2015

This is going to make a lot of people very happy. I'll test this locally and get it merged.

Nice find!

@shafreeck shafreeck mentioned this pull request Aug 7, 2015
Closed
@shafreeck
Copy link
Contributor Author

shafreeck commented Aug 11, 2015

@michael-grunder
I am glad to see the php bug #70198 has been fixed though without merging my PR. I am wondering how is your test going ?

@shafreeck
Copy link
Contributor Author

shafreeck commented Aug 17, 2015

@michael-grunder Now my mate @git-hulk has proofed that it is exactly has this problem by throwing 'read error on connection' exception with errno which showed it is EAGAIN(11).

thanks to @git-hulk !

@shafreeck
Copy link
Contributor Author

shafreeck commented Oct 28, 2015

It takes too long too to merge this pr. @michael-grunder I'd like to help if you have some problems.

@michael-grunder michael-grunder merged commit c18d58b into phpredis:develop Nov 18, 2015
@michael-grunder
Copy link
Member

michael-grunder commented Nov 18, 2015

Many apologies for the long delay. Tested and worked great. Merged!

@parhamr
Copy link

parhamr commented Nov 18, 2015

Apologies for the skepticism, but I want actual proof that this fixes #70 (e.g. scrollback that shows correct behavior in test scripts). It appears there are several contexts where issue #70 was reported, and notably many of the reports were related to PHP-FPM coordination of resources and sockets across multiple processes.

Was this change tested against any of the examples where threading and forking could reproduce the bug?

@michael-grunder
Copy link
Member

michael-grunder commented Nov 18, 2015

Sorry I didn't intend to close #70 by merging this request. It may very well fix the problem for certain cases however. Reopened.

@parhamr
Copy link

parhamr commented Nov 18, 2015

@michael-grunder thanks!

@tom-pryor tom-pryor mentioned this pull request Dec 19, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

debug read error on connection

3 participants

@shafreeck @michael-grunder @parhamr