currentWindow concurrency problem | currentWindow 多线程issue

[PatrickZhangOracle]

else if (windowStart == old.windowStart()) {
return old;
} else if (windowStart > old.windowStart()) {
if (updateLock.tryLock()) {
try {
// Successfully get the update lock, now we reset the bucket.
return resetWindowTo(old, windowStart);
} finally {
updateLock.unlock();
}
} else {
// Contention failed, the thread will yield its time slice to wait for bucket available.
Thread.yield();
}
}

请问以上LeapArray currentWindow方法的代码是否有多线程问题， 第一个线程在resetWindowTo， resetWindowTo有两步，第一步w.resetTo(startTime); 第二步w.value().reset(); 当第一步完成时，另外一个线程正好判断windowStart == old.windowStart()通过，但是value().rest()还没有执行，拿到了没有reset的value，请教一下这里会出现这样的问题吗？

[yanxianchao]

在我看来，这个地方是有问题。WindowWrap 这个类是一个可变类，并且非线程安全，但是却在被多线程访问，有下面两个问题：
（1）属性windowStart和value，没有用volatile进行修饰，无法保障可见性，根据JVM内存模型的规定，读线程理论上可能永远读不到写线程写入的值。
（2）resetTo和value.reset这两个动作，是一个原子操作，但没有保障原子性，线程可以读到一个中间状态，也就是楼主提到的问题。

[yanxianchao]

针对这个问题，我想到两个方案：
方案一：
（1）把WindowWrap改成一个不可变类，对属性windowStart和value加上final修饰，java5之后对final的语义进行了增强，只要对这个对象进行了正确的创建（构造方法中没有溢出这个对象），就可以保证只要读到了这个对象，一定是一个一致性的对象，也就是windowStart和value的设置是一个原子操作，永远看不到这个对象不一致的状态。
（2）修改获取当前窗口的那个方法（com.alibaba.csp.sentinel.slots.statistic.base.LeapArray#currentWindow(long)），当发现当前时间对应到的窗口开始时间大于当前窗口的开始时间时（windowStart > old.windowStart()），执行WindowWrap新建的逻辑（WindowWrap变成了不可变对象），也就是old == null的那段逻辑。

[yanxianchao]

方案二：
（1）把类WindowWrap的两个属性，windowStart和value加上volatile修饰，这一点的改动是必须的，根据jvm内存模型的规则，要保证可见性。
（2）修改抽象方法com.alibaba.csp.sentinel.slots.statistic.base.LeapArray#resetWindowTo的所有实现方法，调整w.resetTo(startTime)和w.value().reset()这两个动作的执行顺序，也就是先resetTo然后再value().reset，进行了value().reset作为windowStart == old.windowStart()的一个前提，反过来说如果进入了windowStart == old.windowStart()这个分支，那么必定已经进行了value().reset，思想有点类似于ConcurrentLinkedQueue的入队操作，对象可能会存在一个中间状态，但是不会导致业务逻辑的错误

[yanxianchao]

这个问题会影响统计精度（把需要统计的数据进行了reset，所以比实际的指标变小了），如果可以的话，我可以做一下修改。

[PatrickZhangOracle]

@sczyh30 I raised this issue because the codes are so clear that the lock only add on update block not add on get block which will result in concurrency issue. But what I concern is this is a very obvious defect ,so I still want to hear this is just a specific design . Could you help confirm this?
If this is really a critical defect, if we add lock on get block , it will definitely downgrade performance. So @yanxianchao's suggestion seems fine.

[soochowcjf]

@sczyh30 我觉得
@yanxianchao 的pr修改是正确的，可以解答一下没有合并的原因么，又或者是其他设计的思考？