CI中关于DB事务处理的设计缺陷

Hex

作者：CFC4N
原文：http://www.cnxct.com/design-flaws-of-codeigniter-framework/


起因：
    在我们线上的某个业务中，使用较老版本的CodeIgniter框架，其中的DB类中，对DB事物处理部分存在着一个设计上的缺陷，或许也算不上缺陷吧。但他却影响了我们生产环境，导致连锁反应。对业务产生较大影响，且不容易排查。这个问题，我在今年的3月中旬，曾向http://codeigniter.org.cn/的站长Hex 报告过，之后，我也忘记这件事情了。直到今天，我们线上业务又一次以为这个问题，害的我又排查一次。具体原因，各位且先听我慢慢说完。(这个问题同样存在于最新版本Version 2.1.0中)

分析：
    以CodeIgniter框架Version 2.1.0为例，在system\database\DB_driver.php的CI_DB_driver类中第58行有个$_trans_status属性。

PHP复制代码

//system\database\DB_driver.php
var $trans_strict        = TRUE;
var $_trans_depth        = 0;
var $_trans_status        = TRUE; // Used with transactions to determine if a rollback should occur
var $cache_on                = FALSE;
 

复制代码

同时，这个类的query方法中，有赋值此属性的代码，见文件306、307行

PHP复制代码

// This will trigger a rollback if transactions are being used
$this->_trans_status = FALSE;

复制代码

这里也给了注释，告诉我们，如果使用了事物处理，那么这属性将成为一个回滚的决定条件。

在520行的事物提交方法trans_complete中，如下代码

PHP复制代码

/**
 * Complete Transaction
 *
 * @access        public
 * @return        bool
 */
function trans_complete()
{
        if ( ! $this->trans_enabled)
        {
                return FALSE;
        }
 
        // When transactions are nested we only begin/commit/rollback the outermost ones
        if ($this->_trans_depth > 1)
        {
                $this->_trans_depth -= 1;
                return TRUE;
        }
 
        // The query() function will set this flag to FALSE in the event that a query failed
        if ($this->_trans_status === FALSE)
        {
                $this->trans_rollback();
 
                // If we are NOT running in strict mode, we will reset
                // the _trans_status flag so that subsequent groups of transactions
                // will be permitted.
                if ($this->trans_strict === FALSE)
                {
                        $this->_trans_status = TRUE;
                }
 
                log_message('debug', 'DB Transaction Failure');
                return FALSE;
        }
 
        $this->trans_commit();
        return TRUE;
}

复制代码

在535行中，如果_trans_status属性如果是false，那么将发生回滚，并且返回false。

在我们的业务代码中，由于程序员疏忽，没有判断trans_complete()方法是否正确执行，直接告诉用户操作成功，但实际上，程序已经向DB下达回滚指令，并未成功更新DB记录。当用户执行下一步操作时，程序又发现相应记录并未更新，又提醒用户上个操作没有完成，通知用户重新执行。如此反复…

排查的过程，也是挺有意思的，起初从PHP代码中，总是不能确定问题所在，并没有把焦点放到trans_complete()方法的返回上。直到后来strace抓包分析，才知道是因为此属性而导致了回滚。

PHP复制代码

22:54:08.380085 write(9, "_\0\0\0\3UPDATE `cfc4n_user_info` SET `cfc4n_user_lock` = 1\nWHERE `cfc4n_user_id` = \'6154\'\nAND `cfc4n_user_lock` = 0", 99) = 99    //执行更新命令
22:54:08.380089 read(9, ":\0\0\1\377\36\4#42S22Unknown column \'cfc4n_user_lock\' in \'where clause\'", 16384) = 62    //不存在字段，SQL执行错误
22:54:08.381791 write(9, "\21\0\0\0\3SET AUTOCOMMIT=0", 21) = 21    //禁止自动提交
22:54:08.381891 read(9, "\7\0\0\1\0\0\0\0\0\0\0", 16384) = 11
22:54:08.382186 poll([{fd=9, events=POLLIN|POLLPRI}], 1, 0) = 0
22:54:08.382258 write(9, "\v\0\0\0\2jv01_roles", 15) = 15
22:54:08.382343 read(9, "\7\0\0\1\0\0\0\0\0\0\0", 16384) = 11
22:54:08.382631 poll([{fd=9, events=POLLIN|POLLPRI}], 1, 0) = 0
22:54:08.382703 write(9, "\22\0\0\0\3START TRANSACTION", 22) = 22   //开始事务处理
22:54:08.401954 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.402043 read(9, "\7\0\0\1\0\0\0\1\0\1\0", 16384) = 11
22:54:08.417773 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.417872 read(9, "\7\0\0\1\0\0\0\1\0\0\0", 16384) = 11
22:54:08.418256 write(9, "[\0\0\0\3UPDATE `cfc4n_user_info` SET `silver` = CAST( silver + (5) as signed )\nWHERE `cfc4n_user_id` = \'6154\'", 95) = 95    //执行其他SQL语句
22:54:08.418363 read(9, "0\0\0\1\0\1\0\1\0\0\0(Rows matched: 1  Changed: 1  Warnings: 0", 16384) = 52    //成功更新，影响条数1.
22:54:08.430212 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.430314 read(9, "\7\0\0\1\0\0\0\1\0\0\0", 16384) = 11
22:54:08.430698 write(9, "B\0\0\0\3UPDATE `cfc4n_user_info` SET `exp` = exp + 26\nWHERE `cfc4n_user_id` = \'6154\'", 70) = 70     //执行其他SQK语句
22:54:08.430814 read(9, "0\0\0\1\0\1\0\1\0\0\0(Rows matched: 1  Changed: 1  Warnings: 0", 16384) = 52    //成功更新，影响条数1.
22:54:08.432130 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.432231 read(9, "\7\0\0\1\0\0\0\1\0\0\0", 16384) = 11
22:54:08.432602 write(9, "\244\0\0\0\3UPDATE `cfc4n_user_quest` SET `rew` = 1, `retable` = retable + 1, `re_time` = 1335797648\nWHERE `cfc4n_user_id` = \'6154\'\nAND `quest_id` = \'300001\'\nAND `rew` = 0", 168) = 168    //执行其他SQK语句
22:54:08.432743 read(9, "0\0\0\1\0\1\0\1\0\0\0(Rows matched: 1  Changed: 1  Warnings: 0", 16384) = 52    //成功更新，影响条数1.
22:54:08.433517 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.433620 read(9, "\7\0\0\1\0\0\0\1\0\0\0", 16384) = 11
22:54:08.433954 write(9, "\t\0\0\0\3ROLLBACK", 13) = 13    //回滚事务 #注意看这里
22:54:08.434041 read(9, "\7\0\0\1\0\0\0\0\0\0\0", 16384) = 11
22:54:08.434914 write(9, "\v\0\0\0\2database_demo", 15) = 15
22:54:08.434999 read(9, "\7\0\0\1\0\0\0\0\0\0\0", 16384) = 11
22:54:08.435342 write(9, "\21\0\0\0\3SET AUTOCOMMIT=1", 21) = 21  //恢复自动提交
22:54:08.435430 read(9, "\7\0\0\1\0\0\0\2\0\0\0", 16384) = 11
22:54:08.436923 write(9, "\1\0\0\0\1", 5) = 5

复制代码

可以看到，在22:54:08.380085时间点处，发送更新SQL语句指令，在22:54:08.380089时间读取返回结果，得到SQL执行错误，不存在字段”cfc4n_user_lock”；22:54:08.381791和22:54:08.382703两个时间点，PHP发送停止“自动提交”与“开始事务处理”指令，在 22:54:08.433954 发送“事务回滚”指令。

配合如上的代码分析，可以清楚的知道，因为“UPDATE `cfc4n_user_info` SET `cfc4n_user_lock` = 1 WHERE `cfc4n_user_id` = ’6154′ AND `cfc4n_user_lock` = 0”这句SQL执行错误，导致$_trans_status属性被设置为FALSE，当代码提交事务时，被trans_complete()方法判断，认为“上一个事务处理”(下面将仔细分析)中存在SQL语句执行失败，决定回滚事务，不提交。

刚刚提到“上一个事务处理”，可能有些朋友不能理解，我们先继续回到代码中来，继续看该属性，同样在trans_complete方法中，542-545行：

PHP复制代码

// If we are NOT running in strict mode, we will reset
// the _trans_status flag so that subsequent groups of transactions
// will be permitted.
if ($this->trans_strict === FALSE)
{
        $this->_trans_status = TRUE;
}

复制代码

也可以很容易的从注释中看明白，设置CI的设计者，为了更严谨的处理 同一个脚本中，存在多个事务时，事务间彼此关系重要，一荣俱荣，一损俱损。这里的trans_strict属性，是个开关，当 trans_strict为false，便是非严格模式，意味着多个事务之间，彼此关系不重要，不影响。当前一个事务中有SQL语句执行失败，影响不到自己。便将_trans_status 设置为TRUE。
毫无疑问，这是个非常周全的考虑。考虑了多个事务之间的关系，保证业务跑在更严谨的代码上。

可是，我们的代码中，错误的SQL语句是执行在事务处理以外的，并不是事务之内。按照我们对事务的认识，可以很清晰的知道，事务之外的SQL相比事务之内的SQL来说，事务之内的SQL更重要，之外的可以允许出错，但事务之内的，务必要正确，不受外界干扰。但CI的框架中，因为一个事务以外的语句执行失败，却导致整个事务回滚…当然，我们的程序员没有对事务提交方法的返回做判断，这也是个问题。

问题已经很清晰了，那么解决方法想必对你来说，是多么的简单。
比如在trans_start方法中，对_trans_status 属性赋值，设置为TRUE，不理会事务外的问题。

PHP复制代码

function trans_start($test_mode = FALSE)
{
        $this->_trans_status = TRUE;
        if ( ! $this->trans_enabled)
        {
                return FALSE;
        }
 
        // When transactions are nested we only begin/commit/rollback the outermost ones
        if ($this->_trans_depth > 0)
        {
                $this->_trans_depth += 1;
                return;
        }
 
        $this->trans_begin($test_mode);
}

复制代码

结束：
在不明白对方设计意图的情况下，不能盲目的定义对方的代码评价，不管程序作者的水平如何。比自己强，也不能盲目崇拜；比自己弱，更不能乱加指责；理解读懂设计意图，学习他人优秀的设计思路、代码风格、算法效率，这才是一个好习惯。当然codeigniter框架是优秀的。

zdkmygod

楼主说的很有道理，我把源码这一块看了一下果然有这个坑。
貌似作者在使用$this->db->query()的时候，都会把trans_status调成false,然后后面的事务提交都会失败。
总结一下，可以这么理解：作者把每一个query()执行之前添加了一个$this->db->trans_start()。这样的话，如果query()执行失败，会导致之后页面的事务都执行后回滚。
要解决这个问题的话，其实最好给query()增加一个是否在事务中的判断，本来trans_depth可以实现的，但是trans_depth这个功能并没有完成。

testabc111

挖个坟
=============
事务开始
start 或 begin
     ||
     \/
各种query操作
     ||
     \/
事务结束
complete 或 off
================
如果不是手动打开或关闭，那“各种query操作”都是事务的一部分，所以在开发的时候尽量不要用complete这种自动判断后得出结果的操作。而是自己判断结果并手动关闭。

燃雲

相信是理解中存有错误。

Strict Mode是指前后的事务有关，由一串小事务组成一个事务组(transaction group)。前面出错，后面便不要继续去，但更前头完成的工作则可让它保留结果，是一种事务组的结构。

非Strict Mode是所有事务皆独立，是另一种事务组的结构。

还有nested的事务，亦是另一种事务组的结构。

若前后的事务之间无关，直接用

PHP复制代码

$this->db->trans_strict(FALSE);

复制代码

设为非Strict Mode即可，Strict Mode并不比non Strict Mode安全。Strict是个名字，并不会变得更严紧。

修改transaction_start并非好主意，新版本CI出现时要进行额外修改维护，忘了修改时又不容易察觉到问题。自找麻烦！更大的影响是无法再使用Strict Mode。

若有编程人员在别处使用了Strict Mode，明明是设了trans_strict是TRUE，为何前面出错，后面竟继续？找问题又要找好半天！


问题出现，程序员没检查执行结果是直接的错误原因，引伸的问题是程序员不明白有不同的“事务组”结构存在。以为明白事务便明白了一切。

这并非任何设计缺陷，是概念未完全搞明白。

jeongee

$this->db->trans_begin();

$this->db->query('AN SQL QUERY...');
$this->db->query('ANOTHER QUERY...');
$this->db->query('AND YET ANOTHER QUERY...');

if ($this->db->trans_status() === FALSE)
{
    $this->db->trans_rollback();
}
else
{
    $this->db->trans_commit();
}
我用这个，我觉得楼上说的很有道理。

太尉天上飞

支持2楼

cfc4n

燃雲 发表于 2012-5-1 10:46
相信是理解中存有错误。

Strict Mode是指前后的事务有关，由一串小事务组成一个事务组(transaction group) ...

正如您所说，本人对事务组的理解存在错误，这点确实。

不过，当一个脚本中，只存在一个事务，但在事务之前，有UPDATE等操作，执行的SQL语句失败了，那么_trans_status属性将被设置为FALSE，但是事务之内的所有语句全部执行正确，但因为事务外的SQL语句错误，导致整个事务被回滚，那么这算设计的缺陷吗？ 这种情况，跟事务组应该没啥关系吧？

或许我给出的解决方案 存在偏差，
将代码更改为

PHP复制代码

 
function trans_start($test_mode = FALSE)
{
        if ($this->trans_strict === FALSE)
        {
                $this->_trans_status = TRUE;
        }
        if ( ! $this->trans_enabled)
        {
                return FALSE;
        }
 
        // When transactions are nested we only begin/commit/rollback the outermost ones
        if ($this->_trans_depth > 0)
        {
                $this->_trans_depth += 1;
                return;
        }
 
        $this->trans_begin($test_mode);
}
 

复制代码

这样您觉得如何？

燃雲

cfc4n 发表于 2012-5-1 17:59
正如您所说，本人对事务组的理解存在错误，这点确实。

不过，当一个脚本中，只存在一个事务，但在事务之 ...

若strict mode是不必要的，干脆转为非strict mode。

若要用strict mode，偶然开个小差，逢场作戏时不想影响大局，不是已有朋友已建议了吗？转用

PHP复制代码

 
$this->db->trans_begin();
 

复制代码

看不明白修改的作用。

PHP复制代码

 
if ($this->trans_strict === FALSE)
{
    $this->_trans_status = TRUE;
}
 

复制代码

$this->trans_strict默认值不是TRUE的吗？变数在哪改变过数值？若程序不特意去设为FALSE，这if有甚么用？是必定不成立的if。若设为FALSE，不就是使用非strict mode。不就是去调用

PHP复制代码

 
$this->db->trans_strict(FALSE);
 

复制代码

若在非strict mode有同样问题，这是CI出错了，应报告通知Ellislab去维护。


自行修改CI要注意后果，不单是怕改错了，日后维护更是大问题。修改要考虑周全，测试保证完全不影响应用系统其它部份，修改内部使用的手册，亦要有机制保证CI升级后的相应修改及测试。应用系统越大越复杂，工作及危险性越高。

cfc4n

燃雲 发表于 2012-5-1 21:08
若strict mode是不必要的，干脆转为非strict mode。

若要用strict mode，偶然开个小差，逢场作戏时不想影 ...

呵呵，这位朋友，你确定你理解我的意思了？ 你真正认真读懂我的意图了？文章的重点是 _trans_status的问题，而不是strict,能否静下心来读明白我的意思之后，再指正我，可否？ 同样感谢你的回复。

燃雲

你明白你的意思吗？你修改的码

PHP复制代码

 
if ($this->trans_strict === FALSE)
{
    $this->_trans_status = TRUE;
}
 

复制代码

当($this->trans_strict === FALSE)成立时，不就是non strict吗？

Non strict时，CI自会处理好$this->_trans_status，不必烦人。


你专心地想着把$this->_trans_status设为TRUE。

我看的是CI在什么时候，为什么原因会把$this->_trans_status设为TRUE及FALSE。


$this->_trans_status是如何设定的？CI要看$this->trans_strict，也就是我不断重覆的说strict mode/non strict mode。


我的心很静，静心方能看到问题的源头，不纠缠在问题发生点。


我看到的是，你意图想前事务成败不影响后事务执行。我的方法是：

Non strict mode，即$this->db->trans_strict(FALSE);


jeongee朋友的方法是：

Manual transaction，即使用$this->db->trans_begin();


你的方法是:

第一次的修改是：在设定strict mode时执行non strict mode。

第二次的修改是：在设定non strict mode时执行non strict mode。


我能说都重覆地说过，看看其它人怎么看吧！

cfc4n

燃雲 发表于 2012-5-2 08:47
你明白你的意思吗？你修改的码

呵呵，我的意图不是 “前事务成败不影响后事务执行”，我的意图是“前面的非事务的成败，不影响下面事务的执行”，问题的原因是前面一个不属于事务处理的SQL语句执行报错了，导致后面事务的执行。

对于我们开发者来说，非事务的重要性肯定没有事务内语句的重要性高，所以，当非事务报错时，仍要保证事务的正确执行，而不受外界的干扰。


我的修改

PHP复制代码

 
 
if ($this->trans_strict === FALSE)
{
    $this->_trans_status = TRUE;
}
 

复制代码

没错，如果开发者重设$this->trans_strict的值，改为false。那么 这里会将$this->_trans_status 设置为TRUE，那么 对于我提到的那种情况，非事务的语句执行报错，将不影响事务的正常执行了。
如果开发者不重设$this->trans_strict的值，那么依旧按照strict mode来执行，依旧会影响。

太尉天上飞

cfc4n 发表于 2012-5-1 17:59
正如您所说，本人对事务组的理解存在错误，这点确实。

不过，当一个脚本中，只存在一个事务，但在事务之 ...

您假设的前面的非事务update操作失败 trans_status = false; 跟下面的事务中的trans_status哪辈子关系？？？？