给JDK提的一个bug(关于AbstractQueuedSynchronizer.ConditionObject)

2022-10-30 09:04:36

1. 背景

之前读JUC的AQS源码，读到Condition部分，我当时也写了一篇源码阅读文章--(AbstractQueuedSynchronizer源码解读--续篇之Condition)[http://www.cnblogs.com/micrari/p/7219751.html]。Doug Lea大师的代码写的很好，整个设计与编码都很优秀。但是我也在最后的思考与总结中指出了Condition有一个缺陷，在于await/awaitNanos/awaitUntil那些方法，在JavaDoc中写了应该要在持有互斥锁的情况下调用，否则通常会抛出IllegalMonitorStateException。确实在AQS的Condition的实现中会抛出异常，但异常是在fullyRelease->release->tryRelease这样的一条方法调用链中被抛出的。而在fullyRelease之前，会做一件事情就是addConditionWaiter。

这显然是有问题的，ConditionObject中的firstWaiter/lastWaiter都是没有被volatile修饰的，它们的可见性是通过锁的获取和释放来保证的。如果有线程因为某些情况实际上没有持有互斥锁，但是调用了ConditionObject的await，尽管可能会因为fullyRelease方法的调用发现未持有互斥锁而抛出IllegalMonitorStateException，但此时可能已经对ConditionObject的内部数据结果造成了永久性破坏。比如可能有些其他正常通过持有锁来await的线程，再也不能被唤醒了。

2. 提bug

不得不说，这个bug是看源码看出的bug。JDK或者我们日常不会遇到的原因是因为我们一直在正确地使用Condition。保证await前要先持锁，保证signal前也要先持锁。但是作为JDK，必须保证库的鲁棒性，也就是在意外情况下同样能够处理，并且不会崩。

AQS的Condition和JVM提供的wait/notify的native实现，效果是很类似的。作为对比使用wait/notify。即便在有线程未持锁的情况下调用wait，会抛出IllegalMonitorStateException,但是原先wait的线程仍然最终可以被唤醒。但是AQS的Condition由于上述逻辑上的一些疏忽，会导致错误的调用抛出期望的异常，但对内部数据结构造成破坏，导致不可靠。

于是，上了Oracle的bugs.java.com。给Oracle提了一个bug，流程不复杂也不简单，还是要填不少bug相关信息，并且要给出测试用例。我写的用例是这样的：

import java.util.concurrent.locks.Condition;

import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantLock;

public class ConditionObjectTest {

    private static volatile int count = 0;

    public static void main(String[] args) throws InterruptedException {

        Lock lock = new ReentrantLock();

        Condition cond = lock.newCondition();

        int loop = 100;

        for (int i = 0; i < loop; i++) {

            new Thread(() -> {

                lock.lock();

                try {

                    count++;

                    cond.await();

                } catch (InterruptedException ignore) {

                } finally {

                    lock.unlock();

                    count--;

                }

            }, "succ-" + i).start();

            new Thread(() -> {

                boolean illegalMonitor = false;

                try {

                    cond.awaitUninterruptibly();

                } catch (IllegalMonitorStateException ignore) {

                    illegalMonitor = true;

                } finally {

                    assert illegalMonitor;

                }

            }, "fail-" + i).start();

        }

        while (count != loop) {

            Thread.yield();

        }

        while (count > 0) {

            lock.lock();

            try {

                cond.signalAll();

            } finally {

                lock.unlock();

            }

        }

    }

}

其实代码很简单，要做的事情就是证明如果在有线程不持有锁调用Condition#await的情况下，最终这些非法调用都会抛出异常。但是那些正常await的线程，却会出现有的怎么也没办法被唤醒，整个程序hang住。

提完之后，Oracle的bus.java.com会显示已经提交到内部了，等内部审核通过后会分配一个bugID。过了没几天我收到邮件称bug被审核过了，分配的ID是JDK-8187408。

3.结果

接下来20多天，Oracle内部人员貌似对我提的这个bug讨论还挺多的。刚开始有人认为这不是bug，Java Doc写了需要持有锁的情况下调用await。我觉得作为JDK，代码实现不能停留在通过Doc来约束的程度啊，作为库来说鲁棒性非常重要，应该能hold住错误的请求并且还屹立不倒。而且我认为这个bug改起来很容易，和signal一样，在方法一开始就先去检查是否持有锁就行了。

还有人称不明白为什么测试用例要打开断言。其实我的测试用例里的断言只是为了证明那些非法请求确实都被抛出异常了。

不过后面人觉得确实是bug，还有个哥们称他发现这个bug貌似早就被引入了。然后翻出了Doug Lea老爷子很早以前有一次改代码的记录,参考链接。

这算是上古时期AQS的代码了，可以看到Doug Lea老爷子当时把checkConditionAccess方法改为了isHeldExclusively。checkConditionAccess原来是会在每个ConditionObject方法(await/signal那些)都被调用的，但是isHeldExclusively却不会在await方法中调用，await方法通过release来判断锁。

所以这就是这个bug的根源，从逻辑上来说确实release里面也包含了判断是否持有互斥锁的逻辑，但实现语义以及鲁棒性却因为这个改动被弱化了很多。