harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Elford, Chris L" <chris.l.elf...@intel.com>
Subject RE: Re: [drlvm][threading] is harmony-1951 a bug or a feature?
Date Tue, 21 Nov 2006 23:10:49 GMT
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.


-----Original Message-----
From: news [mailto:news@sea.gmane.org] On Behalf Of Salikh Zakirov
Sent: Tuesday, November 21, 2006 10:41 AM
To: dev@harmony.apache.org
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
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
(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
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;
-    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
+        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);
     self->state &= ~TM_THREAD_STATE_IN_MONITOR_WAIT;
     self->current_condition = NULL;
@@ -321,6 +347,7 @@ IDATA VMCALL hythread_monitor_notify_all
         return TM_ERROR_NONE;
+    mon_ptr->notify_flag = 1;
         return hycond_notify_all(mon_ptr->condition);   
@@ -347,6 +374,7 @@ IDATA VMCALL hythread_monitor_notify(hyt
         return TM_ERROR_NONE;
+    mon_ptr->notify_flag = 1;
         return hycond_notify(mon_ptr->condition);   

View raw message