5

I'm analyzing the LongAdder algorithm in detail. LongAdder extends the class Striped64 and in that class the essential method is retryUpdate. The following piece of code is taken from this method; in the linked source code it occupies lines 212–222:

try {  // Recheck under lock
  Cell[] rs; int m, j;
  if ( (rs = cells) != null &&
       (m = rs.length) > 0  &&
       rs[j = (m - 1) & h] == null) {
     rs[j] = r;
     created = true;
   }
} finally {
  busy = 0;
}

Question: How can this try block fail?

Note that the array access 

rs[j = (m - 1) & h]

shouldn't throw an IndexOutOfBoundsException because the result of a bitwise-and operation is always less than or equal than the minimum of its integer arguments, hence 0 <= j <= m-1 is within the bounds of the array.

kaya3
  • 47,440
  • 4
  • 68
  • 97
user120513
  • 531
  • 4
  • 12
  • 2
    This appears to be defensive coding. It could fail if the code changes in ways the original developer didn't expect. – Peter Lawrey Nov 07 '18 at 19:56
  • 1
    What Peter said but the form also hints at the mechanics: a "sort of" lock is acquired in `casBusy()` and the convention is to always release the lock in a finally block (`busy = 0` in this case). Following the convention, even though not strictly needed, makes the (structure of the) code easier to read and understand (at least for me). – vanOekel Nov 07 '18 at 20:55

2 Answers2

2

This very much looks like the pattern used with ReentrantLock everywhere else in the jdk code itself. The "pattern" here is that you should always release the lock, even if an Exception occurred, so usually the code is written as:

Lock someLock...

try {
    // use someLock
} finally {
    someLock.unlock();
}

Since cellsBusy(it was renamed from busy) is actually a busy-spin lock, the pattern is sort of the same here. As such:

cellsBusy = 0;

is actually "releasing the lock". So this is not really about failing, as it is about releasing the lock explicitly. I find this a lot easier to read and reason about the code.

Eugene
  • 117,005
  • 15
  • 201
  • 306
  • +1. I think making the code *"a lot easier to read and reason about"* is very likely why it was written this way. Instead of having to check carefully that each operation in the `try` block cannot throw an exception, just wrapping it in `finally` means you don't have to read the `try` block to know that this code will be executed (except in edge cases like a power outage, where not releasing a lock is the least of your problems anyway). Correct code is not as good as *obviously-correct* code. – kaya3 Jan 22 '20 at 22:39
1

This code - and literally any other Java code before version 11 - could fail as a result of the deprecated Thread.stop method being called from another thread. This results in a ThreadDeath error being thrown in the targeted thread, potentially at any time. However, the thread does at least stay alive long enough for the finally block to execute.

The Thread.stop method is deprecated because this behaviour makes it "inherently unsafe":

Why is Thread.stop deprecated?

Because it is inherently unsafe. Stopping a thread causes it to unlock all the monitors that it has locked. (The monitors are unlocked as the ThreadDeath exception propagates up the stack.) If any of the objects previously protected by these monitors were in an inconsistent state, other threads may now view these objects in an inconsistent state. Such objects are said to be damaged. When threads operate on damaged objects, arbitrary behavior can result. This behavior may be subtle and difficult to detect, or it may be pronounced. Unlike other unchecked exceptions, ThreadDeath kills threads silently; thus, the user has no warning that his program may be corrupted. The corruption can manifest itself at any time after the actual damage occurs, even hours or days in the future.

In theory the code could have been written this way as an attempted defense against leaving the object in an invalid state if the thread it is executed in is stopped from another thread. That said, it is very difficult to guarantee a valid state if Thread.stop could be called at any time, and not very common to even attempt to do so, so it's not likely that this was the author's intention. (If it was, the code would probably have a comment explaining it.)

kaya3
  • 47,440
  • 4
  • 68
  • 97
  • we are talking about _internal_ creation of threads here, written by talented people... I highly doubt this has anything to do with `Thread::stop`, at least I very much hope. – Eugene Jan 22 '20 at 22:42
  • 1
    I agree completely. I just thought it was worth answering *"how can this try block fail?"* literally, since you've already answered the implied (and more interesting) question *"why might this code be written in a try block?"*. – kaya3 Jan 22 '20 at 22:46
  • @kaya3: Amazing comment. Your exactness in using words/language is really impressing. I like that very much. Concerning the answers from you and Eugene, I think it's fair to accept Eugene's one, as it's nearer in what I had wanted to ask. – user120513 Feb 22 '20 at 19:57
  • @user120513 I agree, Eugene's answer is the more useful one. – kaya3 Feb 22 '20 at 20:13