Skip to content

Commit

Permalink
6313903: Thread.sleep(3) might wake up immediately on windows
Browse files Browse the repository at this point in the history 
Reviewed-by: rehn, dcubed, rriggs
  • Loading branch information
David Holmes committed Sep 4, 2019
1 parent 4dc79c2 commit 99c17fb
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 155 deletions.
72 changes: 0 additions & 72 deletions src/hotspot/os/posix/os_posix.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -624,78 +624,6 @@ char* os::build_agent_function_name(const char *sym_name, const char *lib_name,
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;
Expand Down
134 changes: 51 additions & 83 deletions src/hotspot/os/windows/os_windows.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,10 @@ static OSThread* create_os_thread(Thread* thread, HANDLE thread_handle,
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;
Expand Down Expand Up @@ -599,7 +602,10 @@ bool os::create_thread(Thread* thread, ThreadType thr_type,
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;
Expand Down Expand Up @@ -3480,87 +3486,7 @@ void os::pd_start_thread(Thread* thread) {
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.
//
Expand Down Expand Up @@ -3687,6 +3613,9 @@ void os::interrupt(Thread* thread) {

ParkEvent * ev = thread->_ParkEvent;
if (ev != NULL) ev->unpark();

ev = thread->_SleepEvent;
if (ev != NULL) ev->unpark();
}


Expand Down Expand Up @@ -5170,6 +5099,40 @@ bool os::ThreadCrashProtection::call(os::CrashProtectionCallback& cb) {
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:
Expand Down Expand Up @@ -5205,7 +5168,7 @@ bool os::ThreadCrashProtection::call(os::CrashProtectionCallback& cb) {
// 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:
Expand Down Expand Up @@ -5278,11 +5241,16 @@ int os::PlatformEvent::park(jlong Millis) {
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;
Expand Down
83 changes: 83 additions & 0 deletions src/hotspot/share/runtime/os.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1836,3 +1836,86 @@ os::SuspendResume::State os::SuspendResume::switch_state(os::SuspendResume::Stat
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 ;
}
}

0 comments on commit 99c17fb

Please sign in to comment.