8000 Work around PHP bug of liveness checking by shafreeck · Pull Request #643 · phpredis/phpredis · GitHub
[go: up one dir, main page]
Skip to content

Work around PHP bug of liveness checking #643

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 18, 2015
Merged

Work around PHP bug of liveness checking #643

merged 1 commit into from
Nov 18, 2015

Conversation

shafreeck
Copy link
Contributor

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

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

@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

@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

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

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

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
0