Fixed a problem with spurious wakeups.

Fixed a problem with spurious wakeups.

Committed: https://code.google.com/p/dart/source/detail?r=597

[jrgfogh, 2011-10-21 11:50:14]

MessageQueue::Dequeue() may return NULL due to spurious wakeups.
I have fixed the problem.

[Søren Gjesse, 2011-10-21 12:19:05]

lgtm

[siva, 2011-10-21 21:18:33]

http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:43: // Wait may wake up prematurely, so we need to
loop until result != NULL
// Wait for message to be enqueued or timeout.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:49: break;
This is not correct, if a spurious wake up happens before the specified time you
are just waking up and returning as
though it timed out.

The right way to do this:
while (result == NULL) {
  WaitResult result = ml.Wait(millis);
  if (result == kTimedOut) {
    return NULL;
  }
  result = head_;
}

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode6
runtime/vm/thread.h:6: #define RUNTIME_VM_THREAD_H_
Why did you change this? If you look at all the other header files you will see
a uniform pattern and we need to stick to that.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode73
runtime/vm/thread.h:73: // May return too early due to spurious wakeups. Callers
should test for this.
I think this comment is not necessary.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode122
runtime/vm/thread.h:122: // May return too early due to spurious wakeups.
Callers should test for this.
Ditto.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode144
runtime/vm/thread.h:144: #endif  // RUNTIME_VM_THREAD_H_
ditto.

[Ivan Posva, 2011-10-21 23:22:56]

Rolled back with http://codereview.chromium.org/8370016/

-Ivan


On 2011/10/21 21:18:33, asiva wrote:
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc
> File runtime/vm/message_queue.cc (right):
> 
>
http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc#new...
> runtime/vm/message_queue.cc:43: // Wait may wake up prematurely, so we need to
> loop until result != NULL
> // Wait for message to be enqueued or timeout.
> 
>
http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc#new...
> runtime/vm/message_queue.cc:49: break;
> This is not correct, if a spurious wake up happens before the specified time
you
> are just waking up and returning as
> though it timed out.
> 
> The right way to do this:
> while (result == NULL) {
>   WaitResult result = ml.Wait(millis);
>   if (result == kTimedOut) {
>     return NULL;
>   }
>   result = head_;
> }
> 
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h
> File runtime/vm/thread.h (right):
> 
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode6
> runtime/vm/thread.h:6: #define RUNTIME_VM_THREAD_H_
> Why did you change this? If you look at all the other header files you will
see
> a uniform pattern and we need to stick to that.
> 
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode73
> runtime/vm/thread.h:73: // May return too early due to spurious wakeups.
Callers
> should test for this.
> I think this comment is not necessary.
> 
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode122
> runtime/vm/thread.h:122: // May return too early due to spurious wakeups.
> Callers should test for this.
> Ditto.
> 
> http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode144
> runtime/vm/thread.h:144: #endif  // RUNTIME_VM_THREAD_H_
> ditto.

[jrgfogh, 2011-10-24 09:20:42]

http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8362026/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:49: break;
On 2011/10/21 21:18:33, asiva wrote:
> This is not correct, if a spurious wake up happens before the specified time
you
> are just waking up and returning as
> though it timed out.
> 
> The right way to do this:
> while (result == NULL) {
>   WaitResult result = ml.Wait(millis);
>   if (result == kTimedOut) {
>     return NULL;
>   }
>   result = head_;
> }

I know. Neither way is strictly correct. My code has the problem you mention.
Your fix may result in a live-lock, since it restarts the wait with the same
timeout each time. The correct solution would be to decrease millis by the
elapsed time. sgjesse and I discussed it and decided that a "correct" solution
would be needlessly complicated, since the caller would have to check for NULL
anyway if 0 != millis.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode6
runtime/vm/thread.h:6: #define RUNTIME_VM_THREAD_H_
On 2011/10/21 21:18:33, asiva wrote:
> Why did you change this? If you look at all the other header files you will
see
> a uniform pattern and we need to stick to that.
A presubmit script told me to do it when I tried to upload the CL. It was based
on some version of lint.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode73
runtime/vm/thread.h:73: // May return too early due to spurious wakeups. Callers
should test for this.
On 2011/10/21 21:18:33, asiva wrote:
> I think this comment is not necessary.
I disagree, since it affects the semantics. I would normally expect a class
Monitor to have correct monitor semantics, which this one doesn't.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode122
runtime/vm/thread.h:122: // May return too early due to spurious wakeups.
Callers should test for this.
On 2011/10/21 21:18:33, asiva wrote:
> Ditto.

Ditto.

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode144
runtime/vm/thread.h:144: #endif  // RUNTIME_VM_THREAD_H_
On 2011/10/21 21:18:33, asiva wrote:
> ditto.
ditto.

[Søren Gjesse, 2011-10-26 12:07:10]

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

http://codereview.chromium.org/8362026/diff/1/runtime/vm/thread.h#newcode6
runtime/vm/thread.h:6: #define RUNTIME_VM_THREAD_H_
On 2011/10/24 09:20:42, jrgfogh wrote:
> On 2011/10/21 21:18:33, asiva wrote:
> > Why did you change this? If you look at all the other header files you will
> see
> > a uniform pattern and we need to stick to that.
> A presubmit script told me to do it when I tried to upload the CL. It was
based
> on some version of lint.

That is annoying. When this error is the only one the presubmit script gives run
it wiht --bypass_hooks to upload anyway.