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

Decreasing the probability of race condition in session lock #5170

Merged
merged 4 commits into from
Jul 6, 2017

Conversation

tianhe1986
Copy link
Contributor

When using redis or memcached session driver, if a web-client perform multiple requests at the same time, two or more requests may get the session lock, and finally some of them would generate "Session: Error while trying to free lock for" error log.

Using Redis::setnx and Memcached::add properly could help to decrease the probability of this race condition.

Signed-off-by: tianhe1986 <w1s2j3229@163.com>
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
@narfbg
Copy link
Contributor

narfbg commented Jul 5, 2017

It is unclear how or why this patch improves anything ... You'll need to elaborate a lot. :)

@tianhe1986
Copy link
Contributor Author

Take redis driver for example.

When two requests with same cookie arrive at the same time, the two processes may also execute the code to here simultaneously, and get $ttl = -2.
Then they continue to execute $this->_redis->setex($lock_key, 300, time()) and would both succeed, resulting in they all get lock.

However, setnx() or set($key, $value, array('nx')) could be used as a lock. In this situation, only one process could succeed. the others would fail. Refer here

@narfbg
Copy link
Contributor

narfbg commented Jul 5, 2017

Ok, that makes sense for Redis, although it can be simplified:

    $result = ($ttl === -2)
        ? $this->_redis->set($lock_key, time(), array('nx', 'ex' => 300))
        : $this->_redis->setex($lock_key, 300, time());

(yes, $result instead of $set_result :))


What about memcache though?

@tianhe1986
Copy link
Contributor Author

memecache also could use add() for locking refer here

Although it also has problem, add() would be better than set() .

I'll modify the code according to your advice :)

Signed-off-by: tianhe1986 <w1s2j3229@163.com>
@@ -326,7 +326,11 @@ protected function _get_lock($session_id)
continue;
}

if ( ! $this->_memcached->set($lock_key, time(), 300))
$result = ($this->_memcached->getResultCode() === Memcached::RES_NOTFOUND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the method params matching ...

$method = ($this->_memcached->getResultCode() === Memcached::RES_NOTFOUND) ? 'add' : 'set';
if ( ! $this->_memcached->$method($lock_key, time(), 300))

$result = ($ttl === -2)
? $this->_redis->set($lock_key, time(), array('nx', 'ex' => 300))
: $this->_redis->setex($lock_key, 300, time());
if ( ! $result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a single empty line prior to this.

Signed-off-by: tianhe1986 <w1s2j3229@163.com>
@narfbg narfbg added this to the 3.1.6 milestone Jul 6, 2017
@narfbg narfbg merged commit 4bda6f7 into bcit-ci:develop Jul 6, 2017
narfbg added a commit that referenced this pull request Jul 6, 2017
…ce_condition

Decreasing the probability of race condition in session lock
narfbg added a commit that referenced this pull request Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants