Page MenuHomePhabricator

Notice: Unable to unserialize: [-1]. Expected ':' but got '1'. in /srv/mediawiki/php-1.28.0-wmf.1/includes/objectcache/RedisBagOStuff.php on line 313
Closed, ResolvedPublic

Description

With 1.28.0-wmf.1 on testwiki:

Notice: Unable to unserialize: [-1]. Expected ':' but got '1'. in /srv/mediawiki/php-1.28.0-wmf.1/includes/objectcache/RedisBagOStuff.php on line 313

Event Timeline

hashar created this task.May 10 2016, 8:04 PM

No trace though :(

demon added a subscriber: demon.May 11 2016, 8:48 PM

Yeah unserialize() only returns false and an E_NOTICE, less than ideal. We need some debugging in here.

Change 288308 had a related patch set uploaded (by Hashar):
RedisBagOStuff::serialize now throws an exception

https://gerrit.wikimedia.org/r/288308

Anomie added a subscriber: Anomie.May 11 2016, 10:28 PM

...

/**
 * @param mixed $data
 * @return string
 */
protected function serialize( $data ) {
    // Serialize anything but integers so INCR/DECR work
    // Do not store integer-like strings as integers to avoid type confusion (bug 60563)
    return is_int( $data ) ? $data : serialize( $data );
}

/**
 * @param string $data
 * @return mixed
 */
protected function unserialize( $data ) {
    return ctype_digit( $data ) ? intval( $data ) : unserialize( $data );
}

It'll store negative integers raw because they're integers, but then try to pass them to unserialize because the - isn't a digit. We might want to just change the latter to something like

protected function unserialize( $data ) {
    $int = intval( $data );
    return $data === (string)$int ? $int : unserialize( $data );
}

This seems to be related to something Echo started doing in 1.28.0-wmf.1, because when I put wmf1 into wmf23 just now, we suddenly started getting these notices from wmf23 too.

I believe this is being triggered by the following code in Echo (NotifUser::getLastUnreadNotificationTime()):

if ( $timestamp === false ) {
        // No notifications, so no timestamp
        $returnValue = false;
        $cacheValue = -1;
} else {
        $returnValue = $timestamp;
        $cacheValue = $timestamp->getTimestamp( TS_MW );
}

$this->setInCache( $memcKey, $cacheValue, 86400 );
return $returnValue;

To indicate a bottom value (i.e. no timestamp) this code sets -1. The cache here is backed by redis, and apparently we weren't writing -1s to redis before.

Change 288343 had a related patch set uploaded (by Catrope):
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288343

Change 288343 had a related patch set uploaded (by Catrope):
RedisBagOStuff: Fix unserialization of negative numbers
https://gerrit.wikimedia.org/r/288343

I turned @Anomie's comment above into a patch.

Change 288343 merged by jenkins-bot:
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288343

Change 288344 had a related patch set uploaded (by Catrope):
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288344

Change 288344 merged by jenkins-bot:
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288344

Change 288346 had a related patch set uploaded (by Catrope):
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288346

Change 288346 merged by jenkins-bot:
RedisBagOStuff: Fix unserialization of negative numbers

https://gerrit.wikimedia.org/r/288346

Catrope closed this task as Resolved.May 12 2016, 5:42 AM
Catrope claimed this task.

I deployed this and the notices stopped. There haven't been any more in the past 45 minutes.

Change 288308 abandoned by Hashar:
RedisBagOStuff::serialize now throws an exception

Reason:
Fix is https://gerrit.wikimedia.org/r/#/c/288343/

https://gerrit.wikimedia.org/r/288308

Catrope closed this task as Resolved.May 12 2016, 10:01 PM

Change 288522 had a related patch set uploaded (by Catrope):
Follow-up cdc93a62bf: add serialize/unserialize tests for RedisBagOStuff

https://gerrit.wikimedia.org/r/288522

Change 288522 merged by jenkins-bot:
Follow-up cdc93a62bf: add serialize/unserialize tests for RedisBagOStuff

https://gerrit.wikimedia.org/r/288522