Hello World
Spiga

Sys.StringBuilder里的Bug

2007-02-13 02:35 by 老赵, 6446 visits

他们肯定没有做好Code Review。

Sys.StringBuilder类源码

话不多说,首先我把Sys.StringBuilder类的源代码完整的贴出,然后根据代码来寻找问题:

Sys.StringBuilder = function Sys$StringBuilder(initialText) {
    /// 
    var e = Function._validateParams(arguments, [
        {name: "initialText", mayBeNull: true, optional: true}
    ]);
    if (e) throw e;

    this._parts = 
(typeof(initialText) !== 'undefined' && initialText !== null && initialText !== '') ? [initialText.toString()] : []; this._value = {}; this._len = 0; } function Sys$StringBuilder$append(text) { /// var e = Function._validateParams(arguments, [ {name: "text", mayBeNull: true} ]); if (e) throw e; this._parts[this._parts.length] = text; } function Sys$StringBuilder$appendLine(text) { /// var e = Function._validateParams(arguments, [ {name: "text", mayBeNull: true, optional: true} ]); if (e) throw e; this._parts[this._parts.length] = ((typeof(text) === 'undefined') || (text === null) || (text === '')) ? '\r\n' : text + '\r\n'; } function Sys$StringBuilder$clear() { if (arguments.length !== 0) throw Error.parameterCount(); this._parts = []; this._value = {}; this._len = 0; } function Sys$StringBuilder$isEmpty() { /// if (arguments.length !== 0) throw Error.parameterCount(); if (this._parts.length === 0) return true; return this.toString() === ''; } function Sys$StringBuilder$toString(separator) { /// /// var e = Function._validateParams(arguments, [ {name: "separator", type: String, mayBeNull: true, optional: true} ]); if (e) throw e; separator = separator || '';
var parts = this._parts; if (this._len !== parts.length) { this._value = {}; this._len = parts.length; }
var val = this._value; if (typeof(val[separator]) === 'undefined') { // Need to remove empty elements before joining in the separator case if (separator !== '') { for (var i = 0; i < parts.length;) { if ((typeof(parts[i]) === 'undefined') ||
(parts[i] === '') || (parts[i] === null)) { parts.splice(i, 1); } else { i++; } } } val[separator] = this._parts.join(separator); } return val[separator]; } Sys.StringBuilder.prototype = { append: Sys$StringBuilder$append, appendLine: Sys$StringBuilder$appendLine, clear: Sys$StringBuilder$clear, isEmpty: Sys$StringBuilder$isEmpty, // separator may be null, to match behavior of ECMA Array.join(separator) and // .NET String.Join(separator, value) toString: Sys$StringBuilder$toString } Sys.StringBuilder.registerClass('Sys.StringBuilder');

Sys.StringBuilder类的Bug分析与重现

Sys.StringBuilder方法的实现很简单,只是把每一次append的内容添加到_parts数组里,在toString时调用数组的join方法来生成一个新的字符串,以次避免过多字符串拼接造成的性能损失。其中Bug的关键就在于它的toString方法,我们自己来分析一下它的实现。

toString方法的作用是把separater作为分隔符,连接StringBuilder内已经添加的部分。它的作用和.NET Framework里String.Join(string separator, string[] value)的功能非常相似。首先,如果用户没有提供separater,则默认separator为空字符串。然后判断_len与_parts数组的长度是否相同,如果不同,则表示_parts被修改过了,于是将作为缓存的_value字典清空并保留新的_len值。如果separator不为空字符串,则遍历_parts数组,清除其中所有为空的元素。这个操作不会影响结果,但是能够提高join方法的性能。最后将调用_parts数组的join方法生成一个新的字符串,并以separator作为key保存在_value数组中。这样,如果Sys.StringBuilder没有改变的话,使用相同的separator调用toString方法则会直接从_value字典的缓存中获取,而避免了再次调用join方法造成的性能损失。

这只是我对于这段实现的“意图”的一个理解,因为这段实现有着非常严重的Bug。大家有没有觉得这段代码有些奇怪?如果_len与_parts数组的长度不同时,则会认为_parts数组被改变了(这“似乎”没错)。但是在重新设定了_len的数值之后,接下来的代码又可能修改了_parts数组(用于清空其中的空元素),这样即使Sys.StringBuilder的内容不被外界改变时,_parts数组的长度也变化了,这造成了_len的数值与_parts.length不同,缓存失效,又必须调用join方法了——没错,甚至会重新遍历_parts数组。

下面的代码就直接说明了问题:

var sb = new Sys.StringBuilder();
for (var i = 0; i < 5; i++)
{
	sb.append(i);
	sb.append(null);
}

alert(sb.toString('|'));
alert(sb.toString('|'));

首先,我们向StringBuilder中添加部分空元素,然后连续调用两次toString方法,传入同样的separator。如果跟踪一下代码就会发现,第二次调用toString方法时并没有利用到缓存,而且重新遍历了_parts数组。虽然生成了相同的结果,但是与它设计的本意违背了,它的缓存“无缘无故”地失效了。

如果说,上面的问题只能说明这个Bug使Sys.StringBuilder没有符合Sys.StringBuilder的设计初衷,那么我下面构造的一个场景就表明这个Bug的严重性了,它能够使Sys.StringBuilder的使用得到错误的结果。如下:

var sb = new Sys.StringBuilder();
for (var i = 0; i < 5; i++)
{
	sb.append(i);
}

sb.append(null);
alert(sb.toString('|')); // "0|1|2|3|4"

sb.append('abc');
alert(sb.toString('|')); // 依旧是"0|1|2|3|4"?

很明显,两次alert得到的结果都是"0|1|2|3|4"完全是错误的。我只是简单的利用了toString方法中判断_parts数组有没有被更新的错误方法——怎么可以使用_len记录_parts数组长度的同时又减少了_parts数组的元素呢?在上面的代码中,我向StringBuilder里添加了一个将会被去掉的null元素,然后在调用toString方法之后重新append一个新的元素以“补充”_parts数组。“愚蠢”的StringBuilder这下认为Sys.StringBuilder没有任何改变,从_values字典中读取了被缓存的结果,这自然是错误的。

RC版本中的StringBuilder居然是正确的

RC版本中的Sys.StringBuilder类的实现居然是正确的,这不是天方夜谭,我们来看一下StringBuilder在RC中的做法:

function Sys$StringBuilder$append(text) {
    /// 
    var e = Function._validateParams(arguments, [
        {name: "text", mayBeNull: true}
    ]);
    if (e) throw e;

    if (typeof(text) !== 'undefined' && text !== null && text !== '') {
        this._parts[this._parts.length] = text.toString();
        this._value = {};
    }
}

function Sys$StringBuilder$toString(separator) {
    /// 
    /// 
    var e = Function._validateParams(arguments, [
        {name: "separator", type: String, mayBeNull: true, optional: true}             
    ]);
    if (e) throw e;

    separator = separator || '';

    if (typeof(this._value[separator]) === 'undefined')
        this._value[separator] = this._parts.join(separator);

    return this._value[separator];
}

多简单优雅的实现,每当调用append方法时,将会直接判断text的有效性(而不是在调用toString方法时去除无效元素),在确认有效之后则添加至_parts数组中,然后将_value字典,也就是把缓存清空。在调用toString方法时,则不需要去除parts数组中的无效元素,也可以非常合理的利用缓存。可以看出,在RC中,并没有_len的作用。

那么为什么在RTM时Sys.StringBuilder反而被修改了呢?在这里,我指的是为什么去“修改”,而不是为什么“改错了”。我认为,他们只是为了避免在调用append方法时将_value字典设为新的对象吧。按照RC中的实现,如果调用了1000次append方法,则会生成1000个多余的对象,这的确是种浪费。但是,他们只要这样改不就可以了吗?如下:

function Sys$StringBuilder$append(text) {
    if (typeof(text) !== 'undefined' && text !== null && text !== '') {
        this._parts[this._parts.length] = text.toString();
        this._value = null;
    }
}

function Sys$StringBuilder$toString(separator) {
    separator = separator || '';

    if (!this._value) this._value = {};
    if (typeof(this._value[separator]) === 'undefined')
        this._value[separator] = this._parts.join(separator);

    return this._value[separator];
}

还好,现在的Sys.StringBuilder的Bug并不是那么容易遇到的,在整个Microsoft AJAX Library的代码中还没有遇到过提供separator的toString方法调用。不过这一点并不能掩盖这个错误,如果您需要使用Sys.StringBuilder的话,也请注意一下这个Bug。

总之,在Sys.StringBuilder问题上,他们一定没有做好Code Review。

Creative Commons License

本文基于署名 2.5 中国大陆许可协议发布,欢迎转载,演绎或用于商业目的,但是必须保留本文的署名赵劼(包含链接),具体操作方式可参考此处。如您有任何疑问或者授权方面的协商,请给我留言

Add your comment

30 条回复

  1. 老赵
    admin
    链接

    老赵 2007-02-13 02:44:00

    这个Bug让人啼笑皆非啊。还好的确不是能够轻易遇到的。

  2. 丹心猪(Dansinge)
    *.*.*.*
    链接

    丹心猪(Dansinge) 2007-02-13 09:08:00

    终于有一次凳子了

  3. 丹心猪(Dansinge)
    *.*.*.*
    链接

    丹心猪(Dansinge) 2007-02-13 09:09:00

    这个发现的好

  4. 老赵
    admin
    链接

    老赵 2007-02-13 09:17:00

    @丹心猪(Dansinge)
    相信他们肯定也能够很轻易的看出来,赫赫。

  5. Ame
    *.*.*.*
    链接

    Ame 2007-02-13 09:45:00

    studying...

  6. 老赵
    admin
    链接

    老赵 2007-02-13 10:00:00

    @Ame
    :)

  7. Wisdom-zh
    *.*.*.*
    链接

    Wisdom-zh 2007-02-13 10:10:00

    一时马虎了...

  8. 老赵
    admin
    链接

    老赵 2007-02-13 11:08:00

    @Wisdom-zh
    不该阿……

  9. 碳碳
    *.*.*.*
    链接

    碳碳 2007-02-13 11:56:00

    充分体现出Code Review的重要性

  10. yunhuasheng
    *.*.*.*
    链接

    yunhuasheng 2007-02-13 12:26:00

    关注...

  11. 老赵
    admin
    链接

    老赵 2007-02-13 12:59:00

    @yunhuasheng
    关注什么呢?:)

  12. 老赵
    admin
    链接

    老赵 2007-02-13 12:59:00

    @碳碳
    是啊,可是有多少人怎么做?赫赫。

  13. 大剑师
    *.*.*.*
    链接

    大剑师 2007-02-13 15:01:00

    高,实在是高,这样的错误也能揪出来?你肯定用了

  14. U2U
    *.*.*.*
    链接

    U2U 2007-02-13 16:06:00

    佩服作者!

  15. 老赵
    admin
    链接

    老赵 2007-02-13 16:11:00

    @大剑师
    其实没有用过哎。我只是在重新看代码,因为我之前几乎看过所有的这些代码,所以尤其是修改过的地方我会着重比较一下。这个错误还是很容易看出的。:)

  16. 老赵
    admin
    链接

    老赵 2007-02-13 16:13:00

    @U2U
    :)

  17. 阿福[匿名][未注册用户]
    *.*.*.*
    链接

    阿福[匿名][未注册用户] 2007-02-13 17:04:00

    啥语言啊?下次加个标题吧,完全不懂呢!

  18. 老赵
    admin
    链接

    老赵 2007-02-13 17:35:00

    @阿福[匿名]
    JavaScript,我想这个应该无所谓吧。:)

  19. 孤叶(学习.net框架)
    *.*.*.*
    链接

    孤叶(学习.net框架) 2007-02-13 22:06:00

    GOOD VERY GOOD
    学习了。。

  20. 老赵
    admin
    链接

    老赵 2007-02-13 22:21:00

    @孤叶(学习.net框架)
    :)

  21. JesseZhao
    *.*.*.*
    链接

    JesseZhao 2007-02-14 19:19:00

    这种也能发现,真是佩服啊

  22. 老赵
    admin
    链接

    老赵 2007-02-14 23:20:00

    @JesseZhao
    看代码而已。:)

  23. webabcd
    *.*.*.*
    链接

    webabcd 2007-02-16 21:06:00

    我是你的一个默默的支持者,从你的blog学到很多,十分感谢!
    值此春节之际,祝你新年快乐,全家幸福,生活顺心,感情如意,工作中能充分体现自己的价值,百忙中也能挤出时间写出更多精彩的文章让更多的人从中受益。

    此致敬礼

  24. 老赵
    admin
    链接

    老赵 2007-02-16 23:18:00

    @webabcd
    谢谢您的支持:)

  25. 阿瑞
    *.*.*.*
    链接

    阿瑞 2007-02-27 09:56:00

    佩服老赵的研究精神,支持,:)

  26. 老赵
    admin
    链接

    老赵 2007-02-27 10:28:00

    @阿瑞
    :)

  27. aldebaran
    *.*.*.*
    链接

    aldebaran 2007-03-06 14:11:00

    原因:
    1 可能是A程序员认为自己代码的效率比B程序员好
    2 重写比修改要快(自己的代码总是比较好看的)
    3 没有很好的沟通,不知道以前有同伴写过这样的代码

    我经常会遇到这种问题,请问老赵,软件工程是怎样解决这样的问题的?仅靠自己的自觉性吗?

  28. 老赵
    admin
    链接

    老赵 2007-03-06 15:13:00

    @aldebaran
    其实应该就是Code Review工作没有做好的缘故。不应该某段代码只有编写者知道就应用到产品环境中。

  29. jwhal
    *.*.*.*
    链接

    jwhal 2008-12-11 14:16:00

    你这是c++里的吧,C#里好你不会噢

  30. chenjian[未注册用户]
    *.*.*.*
    链接

    chenjian[未注册用户] 2008-12-17 09:40:00

    学习中。。。。。。。。。。。。

发表回复

登录 / 登录并记住我 ,登陆后便可删除或修改已发表的评论 (请注意保留评论内容)

昵称:(必填)

邮箱:(必填,仅用于Gavatar

主页:(可选)

评论内容(大于5个字符):

  1. Your Name yyyy-MM-dd HH:mm:ss

使用Live Messenger联系我