Hi there,

I think the issue is still true when the cache type is redis right now. Pretty much same root cause, the code writes like this: In the RedisBagOStuff.php file:

	 * Non-atomic implementation of incr().
	 *
	 * Probably all callers actually want incr() to atomically initialise
	 * values to zero if they don't exist, as provided by the Redis INCR
	 * command. But we are constrained by the memcached-like interface to
	 * return null in that case. Once the key exists, further increments are
	 * atomic.
	 * @param string $key Key to increase
	 * @param int $value Value to add to $key (Default 1)
	 * @return int|bool New value or false on failure
	 */
	public function incr( $key, $value = 1 ) {
		list( $server, $conn ) = $this->getConnection( $key );
		if ( !$conn ) {
			return false;
		}
		try {
			if ( !$conn->exists( $key ) ) {
				return null;
			}
**//			// @FIXME: on races, the key may have a 0 TTL//**
			$result = $conn->incrBy( $key, $value );
		} catch ( RedisException $e ) {
			$result = false;
			$this->handleException( $conn, $e );
		}

		$this->logRequest( 'incr', $key, $server, $result );
		return $result;
	}

So the race condition is when it checks it does have the key, and then the key expired before it actually calls incrBy. And the default expiration for incrBy when the key is not present is -1, which means it's not going to expire.
There are plenty solution online to work around this by either lua script or making it atomic through "Multi" command.

In T22595#6398771, @Jianmingchn wrote:

Hi there,

I think the issue is still true when the cache type is redis right now. Pretty much same root cause, the code writes like this: In the RedisBagOStuff.php file:

	 * Non-atomic implementation of incr().
	 *
	 * Probably all callers actually want incr() to atomically initialise
	 * values to zero if they don't exist, as provided by the Redis INCR
	 * command. But we are constrained by the memcached-like interface to
	 * return null in that case. Once the key exists, further increments are
	 * atomic.
	 * @param string $key Key to increase
	 * @param int $value Value to add to $key (Default 1)
	 * @return int|bool New value or false on failure
	 */
	public function incr( $key, $value = 1 ) {
		list( $server, $conn ) = $this->getConnection( $key );
		if ( !$conn ) {
			return false;
		}
		try {
			if ( !$conn->exists( $key ) ) {
				return null;
			}
**//			// @FIXME: on races, the key may have a 0 TTL//**
			$result = $conn->incrBy( $key, $value );
		} catch ( RedisException $e ) {
			$result = false;
			$this->handleException( $conn, $e );
		}

		$this->logRequest( 'incr', $key, $server, $result );
		return $result;
	}

So the race condition is when it checks it does have the key, and then the key expired before it actually calls incrBy. And the default expiration for incrBy when the key is not present is -1, which means it's not going to expire.
There are plenty solution online to work around this by either lua script or making it atomic through "Multi" command.

Please create a new task if there is still a problem (against MediaWiki-libs-BagOStuff) and not reopen a task closed 8 years ago.