Shock!I unexpectedly found a problem with the JDK source

Posted by cbrian on Wed, 13 Nov 2019 02:40:36 +0100

Thoughts on Source Reading

Recently looking at the source of the thread pool under the concurrent package, I found a problem with the JDK source when I saw the ThreadPoolExecutor class.The following is the code snippet for the addWorker method of the ThreadPoolExecutor class:

boolean workerStarted = false;
boolean workerAdded = false;
Worker w = null;
try {
    w = new Worker(firstTask);
    final Thread t = w.thread;
    if (t != null) {
        final ReentrantLock mainLock = this.mainLock;
        mainLock.lock();
        try {
            int rs = runStateOf(ctl.get());

            if (rs < SHUTDOWN ||
                (rs == SHUTDOWN && firstTask == null)) {
                if (t.isAlive()) // precheck that t is startable
                    throw new IllegalThreadStateException();
                workers.add(w);
                int s = workers.size();
                if (s > largestPoolSize)
                    largestPoolSize = s;
                workerAdded = true;
            }
        } finally {
            mainLock.unlock();
        }
        if (workerAdded) {
            t.start();
            workerStarted = true;
        }
    }
} finally {
    if (! workerStarted)
        addWorkerFailed(w);
}
return workerStarted;

The functionality of this code is perfectly fine, but if you use defense statements, the code will be more readable.So what is a defense statement?

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

What is a defense sentence?

Conditional expressions usually have two forms, the first is that all branches are normal behavior, the second is that only one of the answers provided by conditional expressions is normal behavior, and the other is uncommon.These two types of conditional expressions have different uses, which should be demonstrated through code.

If both branches are normal behavior, a conditional expression such as if...else... should be used; if a condition is extremely rare, it should be checked separately and returned from the function immediately if it is true.Such a separate check is often called a guard clauses.Just look at the dry concept, which is not easy to understand. Let's take two examples.

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

Condition Check Replacement

This is a method of calculating employee salaries, in which the salaries of expatriate and retired employees are treated with special rules.These situations are rare, but they do occur occasionally.

public double getSalary() {
    double result;
    if (this.isSeparated) {//Overseas employees
        result = this.separatedSalary();
    } else {
        if (this.isRetired) {//Retired employees
            result = this.retiredSalary();
        } else {//Normal Employee
            result = this.normalSalary();
        }
    }
    return result;
}

In this code, abnormal checks mask normal checks, so you should replace these condition checks with defense statements to improve program clarity.For each check, place a guard statement.A guard statement either returns from a function or throws an exception.

public double getSalary() {
    if (this.isSeparated) {//guard clause
        return this.separatedSalary();
    }
    if (this.isRetired) {//guard clause
        return this.retiredSalary();
    }
    return this.normalSalary();
}

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

Inverse Conditional Substitution

This is a known method for calculating the volume of a cube with a length, width and height, but it has a special requirement: when the height is greater than 0, print Wanfei Society.(Surprised?Not unexpected?Not abrupt?No change?Yes, sometimes that's what we get.)The code looks like this:

public double getVolume(double length, double width, double height) {
    double result = 0.0;
    if (height > 0.0) {
        System.out.println("Wancat Society");
        if (length > 0.0 && width > 0.0) {
            result = length * width * height;
        }
    }
    return result;
}

It's better to replace the condition check with a defense statement, but we need to reverse the corresponding condition, that is, do a logical nonoperation.

public double getVolume(double length, double width, double height) {
    if (height <= 0.0) {//Defense sentence,! (height > 0)
        return 0.0;
    }
    System.out.println("Wancat Society");
    if (length <= 0.0 || width <= 0.0) {//Defense sentence,! (length > 0 & & width > 0)
        return 0.0;
    }
    return length * width * height;
}

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

Why use defense statements?

The essence of defense sentences is to give special attention to a branch.If you use the if...else...structure, you place equal emphasis on the if branch and the else branch.The message that this code structure conveys to the reader is that the branches are equally important.

The defense sentence is different, it tells the reader: "This is very rare, if it really happens, do some necessary collation work, and then quit."If you are no longer interested in the rest of the method, you should of course quit immediately.Guiding the reader of the code to a useless else section only hinders their understanding.After using the guard statement, the code is easier to understand and maintain.

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

Optimize JDK code

Looking at the instructions above, the code snippet of the addWorker method can be optimized to:

boolean workerStarted = false;
boolean workerAdded = false;
Worker w = null;
try {
    w = new Worker(firstTask);
    final Thread t = w.thread;
    if (t == null) {//guard clause
        return workerStarted;
    }

    final ReentrantLock mainLock = this.mainLock;
    mainLock.lock();
    try {
        int rs = runStateOf(ctl.get());

        //guard clause
        if (rs > SHUTDOWN || (rs == SHUTDOWN && firstTask != null)) {
            return workerStarted;
        }
        if (t.isAlive())// precheck that t is startable
            throw new IllegalThreadStateException();
        workers.add(w);
        int s = workers.size();
        if (s > largestPoolSize)
            largestPoolSize = s;
        workerAdded = true;
    } finally {
        mainLock.unlock();
    }
    if (workerAdded) {
        t.start();
        workerStarted = true;
    }

} finally {
    if (!workerStarted)
        addWorkerFailed(w);
}
return workerStarted;

Is it easier to understand the modified code?It is easier to modify the code if new features are added.

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

epilogue

This JDK source is functionally sound and the architecture is perfect, but I think it can be optimized for readability.Code for nested conditional expressions like this is more than one place in the JDK source code, probably because the author did not consider using defensive statements at the time, or was not as critical as I was.We hope that you can replace nested conditional expressions with guard statements in your own coding process to improve code clarity and maintainability.

Welcome to the WeChat Public Number: Wancat Society, which shares Java technology drinks once a week.

Reference: Refactoring: Improving the Design of Existing Code

Topics: Java JDK REST