Hi all, Since Salikh brings up the subject of spurious wakeups, I'm not sure that any of these tests are written to adequately avoid hangs on all possible compliant VM implementations.
The way I read test3.java, if the meToWait thread gets a spurious wakeup to Test2.lock.wait() before main has finished its initial synchronized block, it is possible that the synchronized block in main will never complete as it will never be able to get the lock back from the meToWaitThread. Now one could correct for that by replacing the sleep(10) with a Test2.lock.wait(10). Or if one wanted a less subtle testcase, putting all the wait() calls into loops as suggested by the java/lang/Object API specification. In general, I agree that spurious wakeups are something to be avoided by the implementation where possible [personally, I'd like to see them disallowed]. However, given that the spec allows spurious wakeups, it could actually be a valuable development tool to have an option to insert them extensively to help developers identify places where they have inadequately protected themselves against true spurious wakeups. Therefore, I could see this partially as a feature. Having said that, I support the default behavior for an implementation to notify only the target thread instead of all threads. Chris -----Original Message----- From: news [mailto:[EMAIL PROTECTED] On Behalf Of Salikh Zakirov Sent: Tuesday, November 21, 2006 10:41 AM To: [email protected] Subject: Re: [drlvm][threading] is harmony-1951 a bug or a feature? Artem Aliev wrote: > Guys, > > I attach a kind of fix for the problem to the JIRA, but I'm not sure I > want to commit such fix. > Please review and comment Thanks to Artem for clear explanation of the behaviour of the Test3. (see HARMONY-1951 comments). Formally speaking, the Java specification does not guarantee that Test3.java from HARMONY-1951 will work correctly, as specification allows for "spurious" wake ups from wait(). Thus, the behaviour of the test is still within compliant with the spec. Still, I agree that it would be nice to fix interruption() to prevent spurious wakeups. Regarding the fix proposed by Artem, I think it is overly complex, and does many things that are not required by the specification. I'm thinking about a simpler fix using a state variable in HyThreadMonitor. (which is conveniently protected by monitor mutex) It looks like it fixes the Test3, but I need to test it some more. diff --git vm/thread/src/thread_native_fat_monitor.c vm/thread/src/thread_native_fat_monitor.c index 7e812a2..9994c22 100644 --- vm/thread/src/thread_native_fat_monitor.c +++ vm/thread/src/thread_native_fat_monitor.c @@ -67,6 +67,7 @@ IDATA VMCALL hythread_monitor_init_with_ mon->flags = flags; mon->name = name; mon->owner = 0; + mon->notify_flag = 0; *mon_ptr = mon; return TM_ERROR_NONE; @@ -205,8 +206,33 @@ IDATA monitor_wait_impl(hythread_monitor self->state |= TM_THREAD_STATE_IN_MONITOR_WAIT; hymutex_unlock(self->mutex); - status = condvar_wait_impl(mon_ptr->condition, mon_ptr->mutex, ms, nano, interruptable); - + do { + apr_time_t start; + mon_ptr->notify_flag = 0; // clear flag before waiting + start = apr_time_now(); + status = condvar_wait_impl(mon_ptr->condition, mon_ptr->mutex, ms, nano, interruptable); + if (status != TM_ERROR_NONE + || mon_ptr->notify_flag || hythread_interrupted(self)) + break; + // we should not change ms and nano if both are 0 (meaning "no timeout") + if (ms || nano) { + apr_interval_time_t elapsed; + elapsed = apr_time_now() - start; // microseconds + nano -= (IDATA)((elapsed % 1000) * 1000); + if (nano < 0) { + ms -= elapsed/1000 + 1; + nano += 1000000; + } else { + ms -= elapsed/1000; + } + if (ms < 0) { + assert(status == TM_ERROR_NONE); + status = TM_ERROR_TIMEOUT; + break; + } + assert(0 <= nano && nano < 1000000); + } + } while (1); hymutex_lock(self->mutex); self->state &= ~TM_THREAD_STATE_IN_MONITOR_WAIT; self->current_condition = NULL; @@ -321,6 +347,7 @@ IDATA VMCALL hythread_monitor_notify_all mon_ptr->notify_flag=1; return TM_ERROR_NONE; #else + mon_ptr->notify_flag = 1; return hycond_notify_all(mon_ptr->condition); #endif } @@ -347,6 +374,7 @@ IDATA VMCALL hythread_monitor_notify(hyt mon_ptr->notify_flag=1; return TM_ERROR_NONE; #else + mon_ptr->notify_flag = 1; return hycond_notify(mon_ptr->condition); #endif }
