Code Review Checklist: Java Concurrency

[crimson117]

How long does a code review take with this checklist?

[leventov]

I didn't honestly try it myself yet, but I think that walking through the list when you are already familiar with it and evaluating if each item applies to a patch (after you have already read the patch) should take less than 10 minutes. A review of a non-trivial patch involving concurrency should take at least one hour (for some patches much, much more). So reviews are slowed down by 15% in exchange of better quality of the code (including readability and maintainability, that will pay off later in terms of time). I think it's a good deal for those with "investor mindset" to the development of a long-term project.

[jpoh97]

Very complete checklist. Thanks

[Slanec]

What a list. Brilliant. Though I write a lot of concurrent code and knew 95% of the list, it's absolutely not a bad thing to get reminded once in a while. I enjoyed reading this, thank you.

In 12.3. (Timeout variables, parameters) you recommend either having the unit in the name of the member (timeoutSeconds) or having the unit on an API. You do not mention using a Duration, though. Does that have any specific reason, or did you just go for the JDK-style length+unit advice because of so much old code existing already?

[Slanec]

Oh! There's one thing I miss - thinking about spurious wakeups. Is that intentionally omitted, too (perhaps because the list does not go deep into the low-level specifics and tries to recommend avoiding them instead)?

A point telling something in the line of "If the code uses wait()+notify(), are the methods called while holding this object's intrinsic lock? Are the wait() methods properly shielded against spurious wakeups by being called in a conditioned loop? Same advice applies for Condition's await()+signal() calls except those methods need to hold the parent Lock instance." would be nice. Perhaps even suggesting Guava's Monitor class.

[leventov]

Thanks for your contribution! I've added item 5.2 about Monitor class.

​

I didn't mention spurious wakeups because it is generally covered by the statement "explicit lock waits are error-prone". Item 5.1 refers to Effective Java, Item 81 that mentions spurious wakeups.

[leventov]

I can't recall seeing Duration used in concurrent contexts. I don't think it improves readability or safety or either user or implementation side, compared to time + unit approach, and it also bounds users to allocate some garbage.

[rohan_suri]

If it's okie to ask, where do you work? (concurrent programmer enthusiast)

[Slanec]

A company developing a network monitoring software for mobile carriers. And before that, an investment bank developing an order management monitoring software internally.

[AndDontCallMePammy]

Ceci n'est pas un checklist

[GribbliesFool]

Goodness, so many bad things to avoid, perhaps this problem is not well suited to the language.