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

Fix for Redis EVAL / EVALSHA #3127

Closed
wants to merge 1 commit into from
Closed

Conversation

atdt
Copy link
Contributor

@atdt atdt commented Jul 7, 2014

phpredis casts nil responses to bool(false), whereas HHVM returns NULL for both
false and nil return values. For example:

<?php
$redis = new Redis();
$redis->connect( '127.0.0.1' );
var_dump( $redis->evaluate( 'return' ) );
var_dump( $redis->evaluate( 'return false' ) );

With Zend / phpredis, the output is:

bool(false)
bool(false)

With HHVM, it's:

NULL
NULL

This patch adds a fix and a test.

phpredis casts nil responses to bool(false), whereas HHVM returns NULL for both
false and nil return values. For example:

    <?php
    $redis = new Redis();
    $redis->connect( '127.0.0.1' );
    var_dump( $redis->evaluate( 'return' ) );
    var_dump( $redis->evaluate( 'return false' ) );

With Zend / phpredis, the output is:

    bool(false)
    bool(false)

With HHVM, it's:

    NULL
    NULL

This patch adds a fix and a test.
@fredemmott
Copy link
Contributor

Thanks - this is up for review. FB: D1422528

wmfgerrit pushed a commit to wikimedia/mediawiki that referenced this pull request Jul 7, 2014
The hhvm redis client returns false instead of null.  This caused
JobQueueRedis to get stuck in an infinite loop.  This works around the
difference by catching null as a signal for no more jobs.

It can be reverted when facebook/hhvm#3127 is
in all versions of hhvm we expect to run MediaWiki.

Bug: 67622
Change-Id: I9bbad42f36a80635097b8e0140b48b6492b2f0f5
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

2 participants