intentions and concurrency

I've just noticed a very dangerous detection in IDEA, any thoughts on this?

Consider this:
if(this.blah != null)
{
Blah myblah = this.blah;
if(myblah != null && xyz)
{
....

I might be mistaken, but in concurrent code, isn't it possible that this.blah becomes null right after the if check? That way you end up with myblah as null, as the second check is required. Yet IDEA will say that it's redundant and suggest it's removed.

Thoughts?

10 comments


The warning about redundancy is actually correct per the JLS, and any concurrent code that relied on such a construct is dangerously broken. Without explicit synchronization, the Java compiler and JVM are perfectly within their rights to do the same redundancy check that the inspection is doing, and not re-fetch this.blah the second time. This may result in different threads seeing different values for this.blah. This is especially likely to happen on multi-processor architectures, where different processors are allowed to cache the value of this.blah without any gaurantee of coherence between the caches. Bang, there's two days of debugging time wasted right there.

(Under different JVM specifications and implementations, marking field blah as volatile may or may not cause the second access of this.blah to refresh the value. It's very complicated, and well worth avoiding in practice. Dunno if IDEA would still show a warning if blah was marked volatile.)

For more information on all of this (and a gauranteed cure for insomnia) google for "Java Memory Model".

--Dave Griffith

0

Well, I know all that. The code construct though is still valid I think. Look at it this way, inside the second if, I'm going to be doing lots of calls to myblah.whatever(), and it should never NPE, so I do want to check if it isn't null. Now, if the field was changed concurrently by another thread, then that's fine, I'd be modifying some old value which is OK in this case. I'm not concerned if I'm working with an old ref, I just want to make sure that it isn't null. Without that second null check, it could be null if I don't add in any synchronization (which I don't want to do, this code is performance critical).

0

Why not drop the first check in this case?

0

Because that's not the issue, the code is how it is, and as far as I know is 'valid'. Sure there are other ways to do it, my concern though is that IDEA is suggesting a dangerous fix that causes subtle errors.

Of course, if someone can convince me that the error happens regardless of what IDEA does, or that it doesn't happen either way, then that's fine too. The focus of what I'm saying is that working code + autofix == non-working code (fill in your own value judgements for working/non-working!)

0

Got it. You're correct. If you don't mind your updates going to an old rev of blah, your code is correct, and the auto-fix could break it. Not sure how to gaurd against this without lobotomizing the (really incredibly useful) constant conditions check, but it should probably be documented there as non-thread-safe (as a couple of IG inspections are, IIRC).

--Dave Griffith

0

Hello Hani,

I've just noticed a very dangerous detection in IDEA, any thoughts on
this?

Consider this:
if(this.blah != null)
{
Blah myblah = this.blah;
if(myblah != null && xyz)
{


1. This code smells badly and I like Idea warning me of this.

2. Without any docs, I would refactor it removing the outer if.

3. If I see a lot of shouting docs warning that the most direst sanctions
will be imposed on anyone touching the code, I might not touch it.

4. If the docs also say why the bad smell is there, I'll leave it alone.

5. The bad smell is still there, but with a lot of patchouli to cover it.

my .02


0

1. IDEA doesn't warn, it just suggests turning it into broken code

2. Removing the outer if would be worse for performance (accessing a variable on the stack is faster than a a field access)

3. It's a perfectly legitimate idiom (even if not in common usage), it just requires that the reader groks it.

4. It's not a bad smell

5. It's not a bad smell

0

3. It's a perfectly legitimate idiom (even if not in common usage), it

just requires that the reader groks it.

That's why I said I would leave it alone if it there are docs explaining
why the code has the two if's.

4. It's not a bad smell

5. It's not a bad smell


Don't you notice the patchoulli? Different noses and all that :)


0

2. Removing the outer if would be worse for performance (accessing a
variable on the stack is faster than a a field access)


The access point is right, but the statement that the code is faster with
two if's may not be true.

The speed change will depend on:

1. The time spent on the first if test [if (blah != null).

2. The time spent on the assignment

If most of the time "blah" is not null, the first "if" is mostly useless
and the code will be slower. Only knowledge of the rest of the code [and
profiling] can tell anything about the speed advantage of this change.


0

Right, of course it depends, just give some credit and assume that I know what I'm doing and that in this case the rest of the code does support my assertion that it's faster with the construct I've shown ;)

0

Please sign in to leave a comment.