Impossible race condition between threads?

[Roquen]

Those links are unrelated to what I’m saying. The new JMM requires (for example) all non-violate writes to be made visible to main memory when a volatile write occurs and the volatile write is a memory-barrier for the compiler. You need to know what you’re doing to play these kinds of games and even if you do I’d suggest avoiding.

[nsigma]

@Roquen

Those links are unrelated to what I’m saying.

Huh?

@Roquen

Remember we’re talking about the compiler seeing the value of a field as being constant across the lifetime of a method … The compiler is performing a perfectly legal transformation which doesn’t consult main-memory.

Those links are exactly about the fact that this is an illegal transformation in this case, because the compiler isn’t allowed to assume the value as constant given the crossing of a memory barrier with another thread that is changing the value.

@Roquen

The new JMM requires (for example) all non-violate writes to be made visible to main memory when a volatile write occurs and the volatile write is a memory-barrier for the compiler.

Absolutely, and you can’t have the JIT compiler caching values without regard to this, or the JMM is pointless - again the subject of those links.

@Roquen

You need to know what you’re doing to play these kinds of games and even if you do I’d suggest avoiding.

Definitely! I said the same earlier. No (or being realistic, minimal) shared state is definitely my preference.

[theagentd]

@nsigma

@theagentd

Thanks for the hints! Does this test deadlock for you as well? http://www.java-gaming.org/?action=pastebin&id=754

@Riven

I can’t explain it either, but using ArrayBlockingQueue(1) instead of using SynchronousQueue got rid of the deadlock

SynchronousQueue would be more accurately equivalent to ArrayBlockingQueue(0)! ie., it has no storage at all.

The issue is the use of offer() vs put() - non-blocking vs blocking. Check the return value of offer() and it should be false. The offer() in Thread 1 does not block and cannot return successfully unless Thread 2 is already in the take() method, and then Thread 1 blocks on a take() that can never be serviced. Thread 2 is doing the reverse, hence deadlock.

The use of an ArrayBlockingQueue with a single storage slot would allow offer() to work.

Ugh… Of course. Can’t code at 5 in the morning…

@Spasi

@nsigma

wouldn’t break the Java Memory Model. Any localvar caching should be invalidated by crossing the memory barrier or that would appear to be a bug?

It doesn’t surprise me that this short example is working. I still believe the bug in the original code is elsewhere (probably in the signalling of task completion) and you’re chasing the rabbit down the wrong hole!

The question then would be, why does the bug go away when counting backwards? A bug elsewhere (e.g. calling run(TaskTree) before the current batch of tasks has completed) would still be problematic in that case. Making totalTasks volatile would also make no difference. Btw, theagentd, have you actually tried that with the original code?

That’s exactly what I did to solve it. I set the atomic counter to taskTree.getNumTasks() directly instead of even having a totalTasks variable, decremented the value by 1 and compared it to 0. I have not encountered the problem after that.

@nsigma

@Roquen

Remember we’re talking about the compiler seeing the value of a field as being constant across the lifetime of a method … The compiler is performing a perfectly legal transformation which doesn’t consult main-memory.

Those links are exactly about the fact that this is an illegal transformation in this case, because the compiler isn’t allowed to assume the value as constant given the crossing of a memory barrier with another thread that is changing the value.

@Roquen

The new JMM requires (for example) all non-violate writes to be made visible to main memory when a volatile write occurs and the volatile write is a memory-barrier for the compiler.

Absolutely, and you can’t have the JIT compiler caching values without regard to this, or the JMM is pointless - again the subject of those links.

This is exactly what I think as well. The bug does seem to be “closed” however?

[Spasi]

[quote=“theagentd,post:63,topic:44753”]
I know. I meant only add volatile to totalTasks and change nothing else.

[quote=“theagentd,post:63,topic:44753”]
It was fixed in 7u6 and only affected the client JVM. On what version are you testing?

[theagentd]

[quote=“Spasi,post:64,topic:44753”]

@theagentd

I know. I meant only add volatile to totalTasks and change nothing else.

Ah, I see. I did not test that since my fix was working. ._.

[quote=“Spasi,post:64,topic:44753”]

@theagentd

It was fixed in 7u6 and only affected the client JVM. On what version are you testing?

I was running on 7u25, but I’ve just now updated to 7u45. I have no idea how I was using such an outdated version…

EDIT: Wait, can we be sure that the fix for volatile fields can be applied to the concurrent collections? Maybe that’s already covered somewhere else but not working?
EDIT2: This bug was “backported” (?) in 7u45: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8018259

[theagentd]

Hmm… I don’t think there’s much more I can do anymore since I haven’t been able to reproduce the original problem for a while. The best I can do is probably to revert the fix I used so the code looks identical to before and see if the error comes back with my current Java version. Unless someone has anything to add I feel pretty satisfied for now at least.

With that said, I’d like to thank everyone here so much for this very interesting discussion!

[Roquen]

@nsigma

Those links are exactly about the fact that this is an illegal transformation in this case, because the compiler isn’t allowed to assume the value as constant given the crossing of a memory barrier with another thread that is changing the value.

One of us is misreading the specification. The linked example has a load-fence (in the method) prior to reading the non-volatile field which tells the compiler that it cannot perform certain transforms (including that it must re-read main-memory) and (probably) must emit some CPU specific instruction(s) to insure memory is consistent. The first snippet in this thread does not contain a barrier. The fact that it calls a method which contains a barrier (in my reading of the spec) is irrelevant. If my reading is incorrect that means that java is very limited in the set of transformation that it can apply because it must “prove” that no called function contains a load-fence/store-fence or full barrier. Generally this is difficult to impossible (consider for instance JNI calls). More importantly such a specification wouldn’t (IMHO) serve a useful purpose and would make implementing the compiler & de-compiler a massive headache.

Even if my reading is incorrect the variable still should be defined volatile so the code’s intent is clear both to the compiler and anyone reading the code in the future (including the author at some point in the future).

(EDIT: funny typo)

[nsigma]

@Roquen

The fact that it calls a method which contains a barrier … is irreverent.

Ah, if only that misprint was what you meant! “Damn, fool, don’t you be dissing me by calling no methods” ;D

@Roquen

If my reading is incorrect that means that java is very limited in the set of transformation that it can apply because it must “prove” that no called function contains a load-fence/store-fence or full barrier.

If it first inlined the method call(s), surely that wouldn’t be that difficult? btw - even if this was single threaded, it would have to prove that no method call changed the inlined value!

@Roquen

Even if my reading is incorrect the variable still should be defined volatile so the code’s intent is clear both to the compiler and anyone reading the code in the future (including the author at some point in the future).

Absolutely! In fact, as I said earlier I reckon the bug at the start of this thread is probably due to improperly paired lock calls, which would explain the simpler test case working OK, and a good reason for making intent clear.

The JVM bug linked to earlier, or rather the thread about it, does highlight that there aren’t (weren’t?) good test cases for the memory model in code output by the JIT compiler. Another reason not to trust edges cases in the JMM too much!