RFR: JDK-6546236 Thread interrupt() of Thread.sleep() can be lost on Solaris due to race with signal handler

[Daniel D. Daugherty]

 > http://cr.openjdk.java.net/~fparain/6546236/webrev.00/

Sorry for the delay. This was a difficult review to perform due to
the code motion between files. My technique was to use the 'udiff'
view for a side-by-side visual compare. Of course, this means I
could easily have missed critical differences... :-)

src/os/bsd/vm/os_bsd.cpp
     Deletes os::sleep(), os::naked_sleep(), os::interrupt() and
     os::is_interrupted().

     bsd << os::sleep >> posix
       << assert(!Bsd::supports_monotonic_clock(), "time moving backwards");
       >> warning("time moving backwards");
           The above diff happens twice. Without at least an assert here
           we will be less likely to see this issue pop-up on specific
           test machines.

     bsd << os::is_interrupted >> posix
       picked up a long comment from the Solaris version

src/os/linux/vm/os_linux.cpp
     Deletes os::sleep(), os::naked_sleep(), os::interrupt() and
     os::is_interrupted().

     linux << os::sleep >> posix
       << assert(!Linux::supports_monotonic_clock(), "time moving 
backwards");
       >> warning("time moving backwards");
           The above diff happens twice. Without at least an assert here
           we will be less likely to see this issue pop-up on specific
           test machines.

     linux << os::is_interrupted >> posix
       picked up a long comment from the Solaris version

src/os/solaris/vm/os_solaris.cpp
     Deletes os::sleep(), os::naked_sleep(), os::interrupt() and
     os::is_interrupted(). Also deletes static os_sleep() and
     os::Solaris::setup_interruptible_already_blocked()

     solaris << os::interrupt >> posix
       << // When events are used everywhere for os::sleep, then this 
thr_kill
       << // will only be needed if UseVMInterruptibleIO is true.
       << if (!isInterrupted) {
       <<   int status = thr_kill(osthread->thread_id(), 
os::Solaris::SIGinterrupt());
       <<   assert_status(status == 0, status, "thr_kill");
           Deleting the above code presumes that you're also going to
           remove the remaining uses of UseVMInterruptibleIO. Should
           that be done with this changeset so things are not out of
           sync.

       << // Bump thread interruption counter
       << RuntimeService::record_thread_interrupt_signaled_count();
           This call was only ever made on Solaris. Is this just a
           counter for when a SIGinterrupt has to be thrown because
           of the UseVMInterruptibleIO stuff?

     Doesn't this deletion make INTERRUPTIBLE_NORESTART_VM_ALWAYS
     unused so it can be deleted from
     src/os/solaris/vm/os_solaris.inline.hpp

     Were you also planning on removing the last vestiges of
     UseVMInterruptibleIO? Or will that be done with another bug?

src/os/posix/vm/os_posix.cpp
     Adds os::sleep(), os::naked_sleep(), os::interrupt() and
     os::is_interrupted().

     > // the interrupted flag into the os::Solaris::Event structure,
         Should this be more generic instead of "Solaris" specific?

Dan


On 2/10/14 8:58 AM, Frederic Parain wrote:
> Greetings,
>
> Please review this fix for JDK-6546236.
>
> A race condition exists in the implementation of
> Thread.sleep() and Thread.interrupt() on the
> Solaris platform. For historical reasons, the
> Solaris implementation of sleep() and interrupt()
> differs from the other Unices (Linux, BSD).
> It uses poll() calls while Linux and BSD are
> using ParkEvents.
>
> This changeset reconciles the Solaris code with
> Linux and BSD code. And instead of having three
> copies of this code, sleep() and interrupt()
> code are moved to the os_posix.cpp from
> os_[solaris|linux|bsd].cpp files.
>
> Note that with this changeset, the Solaris
> implementation of sleep() and interrupt()
> is loosing the support for UseVMInterruptibleIO
> but this is not an issue because this VM
> flag has been deprecated in JDK8 and will be
> removed in JDK9.
>
> The code has been tested with JPRT, vm.quick.testlist
> as well as jdk_lang, jdk_util, jdk_nio and jdk_awt
> (not a random list, all those jdk_* test suites are
> using Thread.sleep() and thread.interrupt()).
>
> The bug description:
> https://bugs.openjdk.java.net/browse/JDK-6546236
>
> The webrev:
> http://cr.openjdk.java.net/~fparain/6546236/webrev.00/
>
> Thank you,
>
> Fred
>