close
Skip to content

Decreasing the probability of race condition in session lock#5170

Merged
narfbg merged 4 commits into
bcit-ci:developfrom
tianhe1986:develop_session_race_condition
Jul 6, 2017
Merged

Decreasing the probability of race condition in session lock#5170
narfbg merged 4 commits into
bcit-ci:developfrom
tianhe1986:develop_session_race_condition

Conversation

@tianhe1986

Copy link
Copy Markdown
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

narfbg commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

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

@tianhe1986

Copy link
Copy Markdown
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

narfbg commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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>
}

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

Copy link
Copy Markdown
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
Copy Markdown
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