CI框架(3 or 4) session锁问题引发的讨论
角色介绍
sskaje:提起疑问者,疑似国人
narfbg:CI作者之一
sskaje:
在CI3和CI4中,用来实现session的redis驱动与memcached驱动关于session key锁的设计是不对的.
估计是sskaje一针见血的表达导致了作者的第一次回复会带有不满情绪..
CI4中,代码老套,虽然更加兼容但是仍然有bug
CI4中关于session锁的循环中的伪代码如下:
if redis::ttl(lock_key) > 0 then sleep 1s continue loop redis::set(lock_key, ttl)
这里sskaje拿出了CI4中的代码,估计是当时CI4中代码还没更新,作者也提到在后来的版本中升级了这段代码.但是作者的方法就是接下来的这段代码,直接kill掉了其他的并发请求.
没错,两个并发请求会同时通过ttl(有效时间)>0的检查,并且同时上锁.并发的请求对session执行写操作可能会导致数据污染,然而
1.并发的session读操作
2.一个请求执行session写操作,多个并发请求执行session读操作
都能正常执行
在CI3的某个提交中,引进了如下逻辑(伪代码):
if redis::ttl(lock_key) > 0 then sleep 1s continue loop if redis::ttl(lock_key) == key-not-exists then redis::setNx(lock_key, ttl) else redis::set(lock_key, ttl)
然而并没有什么卵用反而错的更离谱了
与CI4中的情况一样,都是并发请求可以同时越过ttl>0并且匹配到ttl===-2的条件,因为此时还不存在key.请求1在执行redis::setNx后返回成功,请求2因为已经存在了key而失败了.因此请求2将无法获取到session数据
redis::setNx与redis::set不同点在于只对不存在的key生效,若key已存在则会返回false,然后sskaje给出了简单的解决方案:
redis:
protected function _get_lock($session_id) { // PHP 7 reuses the SessionHandler object on regeneration, // so we need to check here if the lock key is for the // correct session ID. if ($this->_lock_key === $this->_key_prefix.$session_id.':lock') { log_message('error', 'SESSION Redis _get_lock'); return $this->_redis->setTimeout($this->_lock_key, 300); } // 30 attempts to obtain a lock, in case another request already has it $lock_key = $this->_key_prefix.$session_id.':lock'; $attempt = 0; do { $result = $this->_redis->set($lock_key, time(), array('nx', 'ex'=>300)); if (! $result) { sleep(1); continue; } $this->_lock_key = $lock_key; break; } while (++$attempt < 30); if ($attempt === 30) { // in case someone added a key without expiration $ttl = $this->_redis->ttl($lock_key); if ($ttl === -1 || $ttl > 300) { $this->_redis->setTimeout($lock_key, 300); } log_message('error', 'Session: Unable to obtain lock for '.$this->_key_prefix.$session_id.' after 30 attempts, aborting.'); return FALSE; } $this->_lock = TRUE; return TRUE; }
Memcached:
protected function _get_lock($session_id) { // PHP 7 reuses the SessionHandler object on regeneration, // so we need to check here if the lock key is for the // correct session ID. if ($this->_lock_key === $this->_key_prefix.$session_id.':lock') { if ( ! $this->_memcached->replace($this->_lock_key, time(), 300)) { return ($this->_memcached->getResultCode() === Memcached::RES_NOTFOUND) ? $this->_memcached->add($this->_lock_key, time(), 300) : FALSE; } return TRUE; } // 30 attempts to obtain a lock, in case another request already has it $lock_key = $this->_key_prefix.$session_id.':lock'; $attempt = 0; do { if ($this->_memcached->add($lock_key, time(), 300)) { sleep(1); continue; } $this->_lock_key = $lock_key; break; } while (++$attempt < 30); if ($attempt === 30) { // if someone manually adds a key with a very long ttl, this session data would be not accessible by clients log_message('error', 'Session: Unable to obtain lock for '.$this->_key_prefix.$session_id.' after 30 attempts, aborting.'); return FALSE; } $this->_lock = TRUE; return TRUE; }
然后给出了2点关于memcahed的建议,是因为memcahed没有提供读取key的ttl的api的问题,因为我用的是redis就略过啦~然后是作者不满的回复,因为你一来就直接说我代码写的不好有bug.
narfbg(作者):
这是非常,非常不可理喻的讨论
首先,你在CI3的仓库中讨论CI4的代码没有任何意义,只会让其他人感到疑惑,无法理解
其次,你没有给出关于错误所在与预期结果的问题陈诉,我只觉得你在说一些关于TTL和返回值的事(当然了,这些是与问题有关系的,但都是实现细节的事情)
最后,你提供了一些简短的伪代码之后就贴了一大段方法的代码上来.是想让其他人直接复制粘贴到自己项目中去而不展示一下跟之前代码的不同之处吗?
请详细的解释一下,仅仅针对CI3的工作环境下从在什么场景下你期望得到什么结果开始解释吧.
作者表示不懂sskaje在说什么,其实只是来自程序员的报复吧,我觉得说的很明白了
sskaje:
亲爱的narfbg,只是讨论一下,CI3与CI4的session处理程序几乎是一样的.所以我就没有在CI4的仓库里再提PR了.
在讨论清楚后我会在两个项目下都提PR的.
接着sskaje贴了一大段代码表示CI4与3的session处理程序几乎一致,然后被作者diss了..
现在,我应该表达清楚了我们只需要在CI3的项目中讨论这个问题了
在新代码中,2个新的请求,A和B,拥有相同的session_id S,同时运行到这段代码:
if (($ttl = $this->_redis->ttl($lock_key)) > 0) { sleep(1); continue; }
同时检查了lock_key S的ttl并且返回-2,因为此时还未上锁,接下来
$result = ($ttl === -2) ? $this->_redis->set($lock_key, time(), array('nx', 'ex' => 300)) : $this->_redis->setex($lock_key, 300, time());
A与B都返回了-2,所以他们同时来到了这里 setNx
$this->_redis->set($lock_key, time(), array('nx', 'ex' => 300))
如果A先执行,因为设置了nx的原因,B就会失败,反之亦然
if ( ! $result) { log_message('error', 'Session: Error while trying to obtain lock for '.$this->_key_prefix.$session_id); return FALSE; }
B没有办法拿到锁,生成了如下错误日志:
ERROR - 2018-01-17 17:06:31 --> Session: Error while trying to obtain lock for nbss:11075jfvblitou09ic772dm3e8gp11ni ERROR - 2018-01-17 17:06:31 --> Severity: Warning --> session_start(): Failed to read session data: user (path: tcp://redis.xxx.xxx.xxx:6379?prefix=nbss:&weight=1) ........../CodeIgniter/system/libraries/Session/Session.php 127
看下我的伪代码,我定位的问题如下:
1.检查并且执行是错的,如果不存在则加上是对的.
2.redis与memcached的实现面临着同样的问题:如果有人错误的将lock key的ttl设置了很长的时间或者永不过期,那么这个session id将永远无法访问
如果不存在则添加 redsi:
$result = $this->_redis->set($lock_key, time(), array('nx', 'ex'=>300));
在redis中回收错误的lock key:
if ($attempt === 30) { // in case someone added a key without expiration $ttl = $this->_redis->ttl($lock_key); if ($ttl === -1 || $ttl > 300) { $this->_redis->setTimeout($lock_key, 300); } log_message('error', 'Session: Unable to obtain lock for '.$this->_key_prefix.$session_id.' after 30 attempts, aborting.'); return FALSE; }
memcached:没有完美的方法,可以这样写
if ($attempt === 30) { // if someone manually adds a key with a very long ttl, this session data would be not accessible by clients $lock_data = $this->_memcached->get($lock_key); if ($lock_data < time() - 300) { $this->_memcached->delete($lock_key); } log_message('error', 'Session: Unable to obtain lock for '.$this->_key_prefix.$session_id.' after 30 attempts, aborting.'); return FALSE; }
我不明白为什么之前的session锁的写法,也不明白为什么会被修复成这样.
说的很明白了~小题外话,目前版本,也就是sskaje现在质疑的这段代码,是一位叫做tianhe1986的中国程序员提交的,在2017年5月份提出的pr并且在作者的审核下合并了代码.
然后呢,作者引用了sskaje的一些话来回复
narfbg(作者):
现在,我应该表达清楚了我们只需要在CI3的项目中讨论这个问题了
我希望看到的讨论就是针对CI3的实现.你写了这么多,有一大半的内容就是贴出了几个版本中不一样的4行代码..
是我写的基础算法,我当然知道它们很相似,并且这种高级别的解决方案应该被普及.我们现在就3.1分支中最新代码的redis驱动来讨论,只要这个明白了剩下的就好说了.
因为sskaje贴了很多代码来说在这个仓库里讨论就行了,搞的作者很不爽
"A与B都返回了-2,所以他们同时来到了这里 setNx $this->_redis->set($lock_key, time(), array('nx', 'ex' => 300))如果A先执行,因为设置了nx的原因,B就会失败,反之亦然"
据我了解,你希望B请求除了超时是不会失败的.并且你认为导致他失败的原因是资源竞争.而#5170解决的是完全不一样的问题-在同样是资源竞争的情况下,2个请求都成功会有潜在的数据污染的风险.而这才是#5170所解决的
如果我理解的没问题(再一次的),你建议的更改如下:(-表示删除 +表示增加
@@ -335,22 +335,12 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle $attempt = 0; do { - if (($ttl = $this->_redis->ttl($lock_key)) > 0) + if ( ! $this->_redis->set($lock_key, time(), array('nx', 'ex' => '300'))) { sleep(1); continue; } - $result = ($ttl === -2) - ? $this->_redis->set($lock_key, time(), array('nx', 'ex' => 300)) - : $this->_redis->setex($lock_key, 300, time()); - - if ( ! $result) - { - log_message('error', 'Session: Error while trying to obtain lock for '.$this->_key_prefix.$session_id); - return FALSE; - } - $this->_lock_key = $lock_key; break; }
这样的目的是为了消除资源竞争的情况,让B请求永不失败,对吗?
再一次的,我们需要专注于核心问题,何况目前的讨论还是忽略了memcached与不存在过期时间的key的情况下-这又是另一个问题了
这里的核心问题指的是资源竞争应该如何处理的问题,而不是应该直接忽略它的存在.
- 计算机运行过程中，并发、无序、大量的进程在使用有限、独占、不可抢占的资源，由于进程无限，资源有限，产生矛盾，这种矛盾称为竞争（Race）。
- 由于两个或者多个进程竞争使用不能被同时访问的资源，使得这些进程有可能因为时间上推进的先后原因而出现问题，这叫做竞争条件（Race Condition）。
sskaje:
作者同学:
#5170是通过杀掉2个并发请求的其中一个来解决资源竞争问题的,副作用是导致了1个失败请求.
更明智的做法是等到超时或者其他请求释放了锁或者到达了最大尝试次数而不是直接杀掉请求.所以没错,这个简化了的代码是我建议的一部分.长时间/永久有效期的key问题也因为这个简化而暴露出问题.
另外,我曾为前公司设计框架时,引进了一套只读模式session处理程序,因为我的大多数api都需要验证登录状态但都是只读的.如果开启了只读模式,该模式下一旦存储了过期时间后session处理程序将会是无锁的并且不会执行写操作,这在高负载网站中是非常有效的.
我在想哪个能成为默认的session锁模式.
杀掉进程太暴力,这样是bug而不是解决方法;
阻塞等待其他请求,如果其他请求慢也不好;
优化成只读模式太复杂;
无锁你怎么看?
长时间/永久有效期的key表示的是开发人员用了与这个锁的key一样的key,并且设置了长时间有效期,或者设置成了永久有效的.
杀掉请求并不是真正的杀掉,只是在session的read或者write方法重写里返回了false,原文是killin就翻成杀掉了.
narfbg(作者):
#5170是通过杀掉2个并发请求的其中一个来解决资源竞争问题的,副作用是导致了1个失败请求.
没错
更明智的做法是等到超时或者其他请求释放了锁或者到达了最大尝试次数而不是直接杀掉请求.所以没错,这个简化了的代码是我建议的一部分
我也同意
我现在明白你为什么要一直提到#5170了,但要知道-之前只用了setex(),引起了数据污染.最重要的是当时#5170解决了这个问题,我想当时没人想到你的解决方法.就这样,没必要再讨论#5170了.
时间/永久有效期的key问题也因为这个简化而暴露出问题
现在来讨论当有人设置锁大于300s或者永久有效的情况吧
问题是,这是很难发生的,并且没有好的解决方法:
* 忽略并且当做获得了key - 可能导致数据污染
* 忽略并且当做失败 - 可能永远无法获取到key
* 删除/重写 - 操作了其他开发人员创造的数据
另一方面,我不觉得这是由资源竞争的任何一个解决方案引起的问题.老实说,为啥会有人故意去设置一个这么特别的key(一个高熵随机key),那这个人的代码出问题是很正常的了..
另外,我曾为前公司设计框架时,引进了一套只读模式session处理程序,因为我的大多数api都需要验证登录状态但都是只读的
酷,但说句题外话:我希望你说的不是REST API,因为那应该是无状态的并且不应该有session这个概念 :)
如果开启了只读模式,该模式下一旦存储了过期时间后session处理程序将会是无锁的并且不会执行写操作,这在高负载网站中是非常有效的.
不能是无锁的
一个共享的读锁可以同时被多个请求获取,但它始终是个锁-写操作应该在读的期间被阻塞.在CI框架中会存在这些问题:
* 在session开启前,你需要提前申明只读模式(我大概是唯一一个会手动载入session而不用自动载入的人了)
* 请求越多,越有可能有写请求加入队列中.其实从锁设计中受益的正是我们的一部分用户群,他们写了存在很多ajax请求的应用,或者一些长轮询 - 这些正是导致问题的理想环境.我们正是为此而模拟了锁,可能在操作系统级别上能解决这个问题吧
* 有一点你说错了...不是高负荷网站 - 高负荷其实是由多个不同的用户引起的,他们都有着不同的sessionId,都拥有各自的锁
不是说没有用,只是不适合CI
我在想哪个能成为默认的session锁模式
默认一词意味着更多的配置选项 - 所以对不起.只能有一种模式
杀掉进程太暴力,这样是bug而不是解决方法;
你不能将你认为是次优的解分类成bug..bug是非预期的负面影响所以这不是bug.话虽这么说,欢迎更好的解决方案.
阻塞等待其他请求,如果其他请求慢也不好;
所以请优化你的app :)
优化成只读模式太复杂;
同意,如上所述
无锁你怎么看?
不行.绝对不行!
这曾是我们的噩梦,回顾一下CI2中与session相关的问题,90%都是因为无锁而导致的.
引进锁并且强制开启是CI3中重写session类的主要原因,我宁愿去掉整个session类也不愿意让它无锁.我在用户手册中的一整章中都在告诫人们不要试图去掉锁.所以绝不会发生.
作者的态度缓和了很多,然后有理有据让人信服,观点是一定要存在锁,锁在人在,同时有表明了确实在CI中没有更好的解决方案.
之后呢,过了12天,sskaje大概忙去了没有回复.说句题外话:我翻了下sskaje的个人网站,发现有很多文章里有中文..
narfbg(作者):
我已经修复了这个问题,但你说你想在自己PR前讨论一下,要我等你还是我自己来了?
其实还是保持了次优解,我看了CI4中的代码,也与一开始提出来的代码是一致的.
sskaje:
作者同学:不好意思上个礼拜有些忙
我刚刚re-forked了CI3推送了一些代码
https://github.com/sskaje/CodeIgniter/commit/c8e321c488a662ad2f5f191d4642eb4bb0dff919
有些代码风格的问题,会稍后修改
我让锁模式可配置了,因为在我的场景下真的不需要这么多锁并且不能阻塞这些请求
但我还在犹豫让锁等待是否是一个最好的默认方案
请reviwe,如果我觉得没问题的话我会在CI3与4中创建PR
可惜的是,这个请求被sskaje删除了,因为作者拒绝了这端代码.
