6313903: Thread.sleep(3) might wake up immediately on windows
Reviewed-by: rehn, dcubed, rriggs
--- a/src/hotspot/os/posix/os_posix.cpp Tue Sep 03 17:45:02 2019 +0300
+++ b/src/hotspot/os/posix/os_posix.cpp Tue Sep 03 23:42:06 2019 -0400
@@ -624,78 +624,6 @@
return agent_entry_name;
}
-int os::sleep(Thread* thread, jlong millis, bool interruptible) {
- assert(thread == Thread::current(), "thread consistency check");
-
- ParkEvent * const slp = thread->_SleepEvent ;
- slp->reset() ;
- OrderAccess::fence() ;
-
- if (interruptible) {
- jlong prevtime = javaTimeNanos();
-
- for (;;) {
- if (os::is_interrupted(thread, true)) {
- return OS_INTRPT;
- }
-
- jlong newtime = javaTimeNanos();
-
- if (newtime - prevtime < 0) {
- // time moving backwards, should only happen if no monotonic clock
- // not a guarantee() because JVM should not abort on kernel/glibc bugs
- assert(!os::supports_monotonic_clock(), "unexpected time moving backwards detected in os::sleep(interruptible)");
- } else {
- millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC;
- }
-
- if (millis <= 0) {
- return OS_OK;
- }
-
- prevtime = newtime;
-
- {
- assert(thread->is_Java_thread(), "sanity check");
- JavaThread *jt = (JavaThread *) thread;
- ThreadBlockInVM tbivm(jt);
- OSThreadWaitState osts(jt->osthread(), false /* not Object.wait() */);
-
- jt->set_suspend_equivalent();
- // cleared by handle_special_suspend_equivalent_condition() or
- // java_suspend_self() via check_and_wait_while_suspended()
-
- slp->park(millis);
-
- // were we externally suspended while we were waiting?
- jt->check_and_wait_while_suspended();
- }
- }
- } else {
- OSThreadWaitState osts(thread->osthread(), false /* not Object.wait() */);
- jlong prevtime = javaTimeNanos();
-
- for (;;) {
- // It'd be nice to avoid the back-to-back javaTimeNanos() calls on
- // the 1st iteration ...
- jlong newtime = javaTimeNanos();
-
- if (newtime - prevtime < 0) {
- // time moving backwards, should only happen if no monotonic clock
- // not a guarantee() because JVM should not abort on kernel/glibc bugs
- assert(!os::supports_monotonic_clock(), "unexpected time moving backwards detected on os::sleep(!interruptible)");
- } else {
- millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC;
- }
-
- if (millis <= 0) break ;
-
- prevtime = newtime;
- slp->park(millis);
- }
- return OS_OK ;
- }
-}
void os::naked_short_nanosleep(jlong ns) {
struct timespec req;
--- a/src/hotspot/os/windows/os_windows.cpp Tue Sep 03 17:45:02 2019 +0300
+++ b/src/hotspot/os/windows/os_windows.cpp Tue Sep 03 23:42:06 2019 -0400
@@ -497,7 +497,10 @@
OSThread* osthread = new OSThread(NULL, NULL);
if (osthread == NULL) return NULL;
- // Initialize support for Java interrupts
+ // Initialize the JDK library's interrupt event.
+ // This should really be done when OSThread is constructed,
+ // but there is no way for a constructor to report failure to
+ // allocate the event.
HANDLE interrupt_event = CreateEvent(NULL, true, false, NULL);
if (interrupt_event == NULL) {
delete osthread;
@@ -599,7 +602,10 @@
return false;
}
- // Initialize support for Java interrupts
+ // Initialize the JDK library's interrupt event.
+ // This should really be done when OSThread is constructed,
+ // but there is no way for a constructor to report failure to
+ // allocate the event.
HANDLE interrupt_event = CreateEvent(NULL, true, false, NULL);
if (interrupt_event == NULL) {
delete osthread;
@@ -3480,87 +3486,7 @@
assert(ret != SYS_THREAD_ERROR, "StartThread failed"); // should propagate back
}
-class HighResolutionInterval : public CHeapObj<mtThread> {
- // The default timer resolution seems to be 10 milliseconds.
- // (Where is this written down?)
- // If someone wants to sleep for only a fraction of the default,
- // then we set the timer resolution down to 1 millisecond for
- // the duration of their interval.
- // We carefully set the resolution back, since otherwise we
- // seem to incur an overhead (3%?) that we don't need.
- // CONSIDER: if ms is small, say 3, then we should run with a high resolution time.
- // Buf if ms is large, say 500, or 503, we should avoid the call to timeBeginPeriod().
- // Alternatively, we could compute the relative error (503/500 = .6%) and only use
- // timeBeginPeriod() if the relative error exceeded some threshold.
- // timeBeginPeriod() has been linked to problems with clock drift on win32 systems and
- // to decreased efficiency related to increased timer "tick" rates. We want to minimize
- // (a) calls to timeBeginPeriod() and timeEndPeriod() and (b) time spent with high
- // resolution timers running.
- private:
- jlong resolution;
- public:
- HighResolutionInterval(jlong ms) {
- resolution = ms % 10L;
- if (resolution != 0) {
- MMRESULT result = timeBeginPeriod(1L);
- }
- }
- ~HighResolutionInterval() {
- if (resolution != 0) {
- MMRESULT result = timeEndPeriod(1L);
- }
- resolution = 0L;
- }
-};
-
-int os::sleep(Thread* thread, jlong ms, bool interruptable) {
- jlong limit = (jlong) MAXDWORD;
-
- while (ms > limit) {
- int res;
- if ((res = sleep(thread, limit, interruptable)) != OS_TIMEOUT) {
- return res;
- }
- ms -= limit;
- }
-
- assert(thread == Thread::current(), "thread consistency check");
- OSThread* osthread = thread->osthread();
- OSThreadWaitState osts(osthread, false /* not Object.wait() */);
- int result;
- if (interruptable) {
- assert(thread->is_Java_thread(), "must be java thread");
- JavaThread *jt = (JavaThread *) thread;
- ThreadBlockInVM tbivm(jt);
-
- jt->set_suspend_equivalent();
- // cleared by handle_special_suspend_equivalent_condition() or
- // java_suspend_self() via check_and_wait_while_suspended()
-
- HANDLE events[1];
- events[0] = osthread->interrupt_event();
- HighResolutionInterval *phri=NULL;
- if (!ForceTimeHighResolution) {
- phri = new HighResolutionInterval(ms);
- }
- if (WaitForMultipleObjects(1, events, FALSE, (DWORD)ms) == WAIT_TIMEOUT) {
- result = OS_TIMEOUT;
- } else {
- ResetEvent(osthread->interrupt_event());
- osthread->set_interrupted(false);
- result = OS_INTRPT;
- }
- delete phri; //if it is NULL, harmless
-
- // were we externally suspended while we were waiting?
- jt->check_and_wait_while_suspended();
- } else {
- assert(!thread->is_Java_thread(), "must not be java thread");
- Sleep((long) ms);
- result = OS_TIMEOUT;
- }
- return result;
-}
+
// Short sleep, direct OS call.
//
@@ -3687,6 +3613,9 @@
ParkEvent * ev = thread->_ParkEvent;
if (ev != NULL) ev->unpark();
+
+ ev = thread->_SleepEvent;
+ if (ev != NULL) ev->unpark();
}
@@ -5170,6 +5099,40 @@
return success;
}
+
+class HighResolutionInterval : public CHeapObj<mtThread> {
+ // The default timer resolution seems to be 10 milliseconds.
+ // (Where is this written down?)
+ // If someone wants to sleep for only a fraction of the default,
+ // then we set the timer resolution down to 1 millisecond for
+ // the duration of their interval.
+ // We carefully set the resolution back, since otherwise we
+ // seem to incur an overhead (3%?) that we don't need.
+ // CONSIDER: if ms is small, say 3, then we should run with a high resolution time.
+ // Buf if ms is large, say 500, or 503, we should avoid the call to timeBeginPeriod().
+ // Alternatively, we could compute the relative error (503/500 = .6%) and only use
+ // timeBeginPeriod() if the relative error exceeded some threshold.
+ // timeBeginPeriod() has been linked to problems with clock drift on win32 systems and
+ // to decreased efficiency related to increased timer "tick" rates. We want to minimize
+ // (a) calls to timeBeginPeriod() and timeEndPeriod() and (b) time spent with high
+ // resolution timers running.
+ private:
+ jlong resolution;
+ public:
+ HighResolutionInterval(jlong ms) {
+ resolution = ms % 10L;
+ if (resolution != 0) {
+ MMRESULT result = timeBeginPeriod(1L);
+ }
+ }
+ ~HighResolutionInterval() {
+ if (resolution != 0) {
+ MMRESULT result = timeEndPeriod(1L);
+ }
+ resolution = 0L;
+ }
+};
+
// An Event wraps a win32 "CreateEvent" kernel handle.
//
// We have a number of choices regarding "CreateEvent" win32 handle leakage:
@@ -5205,7 +5168,7 @@
// 1. Reconcile Doug's JSR166 j.u.c park-unpark with the objectmonitor implementation.
// 2. Consider wrapping the WaitForSingleObject(Ex) calls in SEH try/finally blocks
// to recover from (or at least detect) the dreaded Windows 841176 bug.
-// 3. Collapse the interrupt_event, the JSR166 parker event, and the objectmonitor ParkEvent
+// 3. Collapse the JSR166 parker event, and the objectmonitor ParkEvent
// into a single win32 CreateEvent() handle.
//
// Assumption:
@@ -5278,11 +5241,16 @@
if (Millis > MAXTIMEOUT) {
prd = MAXTIMEOUT;
}
+ HighResolutionInterval *phri = NULL;
+ if (!ForceTimeHighResolution) {
+ phri = new HighResolutionInterval(prd);
+ }
rv = ::WaitForSingleObject(_ParkHandle, prd);
assert(rv == WAIT_OBJECT_0 || rv == WAIT_TIMEOUT, "WaitForSingleObject failed");
if (rv == WAIT_TIMEOUT) {
Millis -= prd;
}
+ delete phri; // if it is NULL, harmless
}
v = _Event;
_Event = 0;
--- a/src/hotspot/share/runtime/os.cpp Tue Sep 03 17:45:02 2019 +0300
+++ b/src/hotspot/share/runtime/os.cpp Tue Sep 03 23:42:06 2019 -0400
@@ -1836,3 +1836,86 @@
return result;
}
#endif
+
+int os::sleep(Thread* thread, jlong millis, bool interruptible) {
+ assert(thread == Thread::current(), "thread consistency check");
+
+ ParkEvent * const slp = thread->_SleepEvent;
+ // Because there can be races with thread interruption sending an unpark()
+ // to the event, we explicitly reset it here to avoid an immediate return.
+ // The actual interrupt state will be checked before we park().
+ slp->reset();
+ // Thread interruption establishes a happens-before ordering in the
+ // Java Memory Model, so we need to ensure we synchronize with the
+ // interrupt state.
+ OrderAccess::fence();
+
+ if (interruptible) {
+ jlong prevtime = javaTimeNanos();
+
+ assert(thread->is_Java_thread(), "sanity check");
+ JavaThread *jt = (JavaThread *) thread;
+
+ for (;;) {
+ // interruption has precedence over timing out
+ if (os::is_interrupted(thread, true)) {
+ return OS_INTRPT;
+ }
+
+ jlong newtime = javaTimeNanos();
+
+ if (newtime - prevtime < 0) {
+ // time moving backwards, should only happen if no monotonic clock
+ // not a guarantee() because JVM should not abort on kernel/glibc bugs
+ assert(!os::supports_monotonic_clock(),
+ "unexpected time moving backwards detected in os::sleep(interruptible)");
+ } else {
+ millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC;
+ }
+
+ if (millis <= 0) {
+ return OS_OK;
+ }
+
+ prevtime = newtime;
+
+ {
+ ThreadBlockInVM tbivm(jt);
+ OSThreadWaitState osts(jt->osthread(), false /* not Object.wait() */);
+
+ jt->set_suspend_equivalent();
+ // cleared by handle_special_suspend_equivalent_condition() or
+ // java_suspend_self() via check_and_wait_while_suspended()
+
+ slp->park(millis);
+
+ // were we externally suspended while we were waiting?
+ jt->check_and_wait_while_suspended();
+ }
+ }
+ } else {
+ OSThreadWaitState osts(thread->osthread(), false /* not Object.wait() */);
+ jlong prevtime = javaTimeNanos();
+
+ for (;;) {
+ // It'd be nice to avoid the back-to-back javaTimeNanos() calls on
+ // the 1st iteration ...
+ jlong newtime = javaTimeNanos();
+
+ if (newtime - prevtime < 0) {
+ // time moving backwards, should only happen if no monotonic clock
+ // not a guarantee() because JVM should not abort on kernel/glibc bugs
+ assert(!os::supports_monotonic_clock(),
+ "unexpected time moving backwards detected on os::sleep(!interruptible)");
+ } else {
+ millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC;
+ }
+
+ if (millis <= 0) break ;
+
+ prevtime = newtime;
+ slp->park(millis);
+ }
+ return OS_OK ;
+ }
+}