On 11/21/06, Elford, Chris L <[EMAIL PROTECTED]> wrote:
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.
Interesting point. To test this theory, I put a,
"System.out.println("sleep(10)
just completed");" immediately after the "sleep(10);" in the method called
run(). On my laptop, DRLVM executes the println statement endlessly.
However, the main thread has printed, "Both threads are waiting now, will
interrupt the second one". This indidates the main thread has already
entered/exited the synchronized(Test2.lock) {...}. Hmmm...
I then added another print statement, "System.out.println("Just did
Test2.toStop.interrupt();" Of course this new line is immedately after
Test2.toStop.interrupt(); It turns out this print statement appears which
suggests the main thread is sending the interrupt and the child thread is
somehow missing it.
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.
I tried replacing sleep(10); with Test2.lock.wait(); The app no longer
hangs, but the above printf's indicate there may still be a bug(s) in how
drlvm handles interrupts.
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.
Yes! I like this idea. It should not be too hard to add to drlvm when the
time is right.
Having said that, I support the default behavior for an implementation
to notify only the target thread instead of all threads.
I agree. If for no other reason than to reduce confusion. If at a later
time we discover this "feature" is causing unacceptable performance
problems, we can always yank it or put it command line selectable.
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
}
--
Weldon Washburn
Intel Enterprise Solutions Software Division
