JDK-6908596 : Missing memory barriers in Thread interruption logic
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 7,8u121,9
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • OS: generic
  • CPU: generic
  • Submitted: 2009-12-09
  • Updated: 2019-11-12
  • Resolved: 2019-11-12
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbdResolved
Related Reports
Duplicate :  
Relates :  
Description
A posting to the concurrency-interest mailing list drew attention to this piece of the Java Memory Model:

On p341 of JCIP, the Interruption Rule states:
"A thread calling interrupt on another thread happens-before the interrupted thread detects the interrupt (either by having InterruptedException thrown, or invoking isInterrupted or interrupted)"

This is referring to the final bullet point in JLS 3, 17.4.4.

While the exact semantics of this statement are unclear the expert opinion was that it means that the interrupted state of a thread should act like a Java volatile variable: interrupt() will involve a volatile-write, while explicitly checking the interrupt state will involve a volatile-read. (This does not fully define the semantics because it doesn't indicate if you can elide the volatile-write in interrupt() if the thread is already interrupted - but that is not important for this CR.)

In light of the recent memory-barrier issue with the park/unpark code (CR 6822370) the thread interruption was code was examined to see if the basic JMM requirements were being met. We discovered one definite issue:

- in os::interrupted(Thread* t, bool clear_interrupt) there is a missing fence after clearing the interrupt state

There is also some discussion over whether reading the interrupt state requires some additional barriers. Doug Lea writes:

> Stepping back a bit first: If interrupt status were a Java
> volatile, we'd place a MemBarAcquire after each read.
> For writes we'd precede with a MemBarRelease and follow
> with a MemBarVolatile. The mechanics (see for example
> insert_mem_bar in graphKit.cpp, as called in
> the usual case in parse3.cpp) prevent reorderings,
> kill opportunities to reuse reads, and maybe
> generate processor barrier instructions.
> 
> Since osthread._interrupted is VM-level field, we
> can't do that, but can get the same effect.
> 
> In the write case, which is all done at VM-level
> (as in os_solaris.cpp approx line 3950)
> we do need a trailing storeLoad fence inside the
> VM-level interrupted, which we have. We don't need
> any further reordering control since the compiler
> must conservatively treat it as an opaque call, which
> disables any reorderings that could matter here.
> 
> The inlined part of read case (library_calls.cpp),
> manually does the reordering prevention, but doesn't
> necessarily have the reads-kill or barrier-instruction
> effects.
> 
> Officially, it ought to, although the only cases
> I can see where it might make a difference are
> racy to start with.
> 
> Because the inlined forms of Thread.interrupted() may
> branch to opaque call to VM-level, the entire if-else
> context can't be reordered in any interesting way.
> However, I'm not so sure about the case of
> non-clearing form isInterrupted returning true:
>   r = x.aField;
>   if (Thread.currentThread().isInterrupted()) {
>      r = x.aField;  // maybe is optimized away?
>      ...
> 
> Also, on machines that actually need acquire fences
> for volatile reads, like ARM and PPC, they
> ought to be generated, although because interrupt
> polling is racy anyway, it is unlikely to matter much.
> 
> Anyway, for the sake of making sure it is right,
> it would be good to ask someone in c2 if/how we
> can add something like
>   insert_mem_bar(Op_MemBarAcquire, p);
> in  LibraryCallKit::inline_native_isInterrupted()
> ("p" is the read of _interrupted).
>
It is worth noting that for some thread t, where t is not the current thread, that t.interrupt() and t.isInterrupted() both result in the holding of the Threads_lock inside the VM - and consequently there are strong barriers already present there. 

Further the c2 intrinsic is only used for the (in practice rare) case of Thread.currentThread().isInterrupted().

In that light it may suffice to make the following changes in osThread.hpp:

bool interrupted() const  {
  return OrderAccess::load_acquire(& _interrupted) != 0;
}
void set_interrupted(bool z) {
  OrderAccess::release_store_fence(&_interrupted, z ? 1 : 0);
}

and remove the explicit release()/fence()  actions in os::interrupt and os::is_interrupted.

If a change is also needed to the C2 intrinsic then a simple load_acquire should also suffice.

Comments
With JDK-8229516 the interrupted state is moved to being a volatile field in java.lang.Thread and all accesses from the JVM are volatile accesses. Ergo no memory barriers are missing. Closing as a duplicate of JDK-8229516.
12-11-2019

I will look at this in the context of JDK-8229516.
22-08-2019

Use of load_acquire/store_release would be better than adding the fence. Need to re-check the lock-free paths.
18-04-2016

This affects both runtime "os" functions and intrinsics.
18-04-2016

This keeps dropping off the radar! Wasn't fixed in 7, or 8, and now it seems 9!
18-04-2016

ILW = specification violation; no issues found for end-user so far; none = MLH = P4
05-04-2016

SUGGESTED FIX For the missing fence in is_interrupted (this is Linux version): bool os::is_interrupted(Thread* thread, bool clear_interrupted) { assert(Thread::current() == thread || Threads_lock->owned_by_self(), "possibility of dangling Thread pointer"); OSThread* osthread = thread->osthread(); bool interrupted = osthread->interrupted(); if (interrupted && clear_interrupted) { osthread->set_interrupted(false); + OrderAccess::fence(); // consider thread->_SleepEvent->reset() ... optional optimization } return interrupted; } Similarly for Solaris.
09-12-2009

EVALUATION See description
09-12-2009