Why can't notifyAll() be used in a Thread instance?

Question

I'm using a source code analyser that pointed out that notifyAll() should not be called within a Thread instance. I'm trying to articulate this to management, but I can't come up with an explanation. Can someone help me??

BTW, this is inherited code, so I'm not responsible for the design decision!

Curmudgeon

What source code analyzer pointed this out? – Hansel

What do you mean by "within a Thread instance" exactly? Is there something actually wrong with the way the code is calling notifyAll? – Pickaback

A quick google search on "sonar thread notifyall" revieled the following page: ci.ops4j.org/sonar/… It states Internally, the JVM relies on these methods to change the state of the Thread (BLOCKED, WAITING, ...), so calling them will corrupt the behavior of the JVM. – Hansel

@Ferrybig: Yes, I didn't mention that SONAR does say that, but it's still not clear to me why. – Curmudgeon

@David Schwartz: I mean notifyAll() is called inside a class that extends Thread. – Curmudgeon

Answer 1

It sounds like the SONAR message you're seeing is this one:

Methods "wait(...)", "notify()" and "notifyAll()" should never be called on Thread instances squid:S2236

Sonar has a detailed explanation of this:

On a Thread instance, the methods wait(...), notify() and notifyAll() are available only because all classes in Java extend Object and therefore automatically inherit the methods. But there are two very good reasons to not call these methods on a Thread instance:

Doing so is really confusing. What is really expected when calling, for instance, the wait(...) method on a Thread? That the execution of the Thread is suspended, or that acquisition of the object monitor is waited for?

Internally, the JVM relies on these methods to change the state of the Thread (BLOCKED, WAITING, ...), so calling them will corrupt the behavior of the JVM.

Likewise, the advice given in the Java API is:

It is recommended that applications not use wait, notify, or notifyAll on Thread instances.

Internal thread management is done by locking on thread objects. If your own application code locks on threads you may get unexpected or confusing behavior. For instance, when a thread terminates it sends a notification to every thread waiting on it.

In trying to assess the potential for problems I'd look for cases where a thread could receive a notification in error or miss out on a notification due to being used as a lock for application code and as a lock for internal JVM code. (This seems like it could be tedious.) Depending on the code I'd also look for possible consistency problems due to locking on threads as opposed to locking the data structures accessed by threads, and assess whether the locking scheme makes sense. If the code is subclassing Thread I would want to check out if threads get pooled and how.

Mostly this message would make me think, if this code is doing this one bad thing then what else is it possibly doing wrong? A SONAR message is just a hint that it found a bad smell in the code so that you can investigate and see how significant the problem is.

Rufina

I guess my question is, what would the behavior be if this code were left in place? The most we can say is it'd be unexpected? I'd like to be more specific. – Curmudgeon

@user1660256 You'd have to look closely at the specific code and analyze its interactions with thread state transitions. The reason for the warning is that nobody wants to have to do that or to take the risk that they'll do it wrong. Hence the warning. – Pickaback

@user1660256: David is correct. it's hard to say without a very specific example. Threading problems can show up intermittently, or on some platforms and not others. I'd analyze the code and check for cases where a notification could get lost or could be misdirected. – Rufina

It would be better if Sonar would complain about any class that @Overrides Thread.run(). – Flint

@james: it would. fortify complains about any kind of hand-rolled kicking off of threads. – Rufina

Answer 2

You shouldn't be sub-classing Thread. Doing so opens up a host of reasonably well know problems. Using wait/notifyAll is not a great choice since the concurrency libraries were added more than ten years ago but on Thread it is particularly bad because the Thread class also uses wait/notify for its own purposes.

Sixpack