mojo: Allow basic RunLoop to be quit from a posted task.

mojo: Allow basic RunLoop to be quit from a posted task.

Right now, if the runloop is observing handles and a delayed task tries
to quit it, it will block until one handle gets a signal. This CL
ensures that a delayed task can quit the runloop.

R=jamesr@chromium.org

Committed: https://crrev.com/8fd73aa30ad43651bb6dc425ef2156073e1a9377
Cr-Commit-Position: refs/heads/master@{#296378}

[darin (slow to review), 2014-09-20 04:53:47]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2014-09-20 04:53:48]

Can you add a test to run_loop_unittest.cc?

[qsr, 2014-09-22 10:02:46]

On 2014/09/20 04:53:48, darin wrote:
> Can you add a test to run_loop_unittest.cc?

 Done.

 I do not test the change on RunUntilIdle, because it is only addressing the
case of a race that I do not know how to reproduce...

[jamesr, 2014-09-22 22:30:18]

I don't understand the comment about RunUntilIdle().  If I post a task that
attempts to quit and then invoke RunUntilIdle() it should hit the code you've
added, right?

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
File mojo/public/cpp/utility/lib/run_loop.cc (right):

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/lib/run_loop.cc:103: Wait(false);
hmm, i think this would be cleaner if it was structured more like the base/
version, but this seems ok

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/lib/run_loop.cc:115: if (!run_state.should_quit &&
!Wait(true) && delayed_tasks_.empty())
this is getting too hard to follow IMO, especially since the thing that actually
does work here is buried in the middle of the if. why would we want to keep
looping here if we should quit? 

how about:

bool keep_running = true;

do {
  // Try to do one unit of work without blocking and if we did, keep running.
  keep_running = Wait(true);
  if (run_state.should_quit)
    break;
  // Try to do a unit of delayed work.
  keep_running |= DoDelayedWork();
} while (keep_running && !run_state.should_quit);

and have ::DoDelayedWork() return true if it actually ran a task?  then this
loop would continue until nothing happened in Wait(true) and DoDelayedWork()

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
File mojo/public/cpp/utility/tests/run_loop_unittest.cc (right):

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/tests/run_loop_unittest.cc:445:
run_loop.PostDelayedTask(Closure(ClosingTask(&test_pipe.handle1)), 1e5);
hmm, 1e5 microseconds = 100ms if i'm not mistaken. On a noisy system, we could
definitely have gaps of larger than this unaccounted for

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/tests/run_loop_unittest.cc:450: ASSERT_LE(after-before,
5e4);
this is super racy. if you want tests like this we need a way to inject a fake
clock implementation.

why not just assert that we exited (i.e. that the test harness didn't time out?)

[qsr, 2014-09-23 10:40:17]

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
File mojo/public/cpp/utility/lib/run_loop.cc (right):

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/lib/run_loop.cc:115: if (!run_state.should_quit &&
!Wait(true) && delayed_tasks_.empty())
On 2014/09/22 22:30:18, jamesr wrote:
> this is getting too hard to follow IMO, especially since the thing that
actually
> does work here is buried in the middle of the if. why would we want to keep
> looping here if we should quit? 
> 
> how about:
> 
> bool keep_running = true;
> 
> do {
>   // Try to do one unit of work without blocking and if we did, keep running.
>   keep_running = Wait(true);
>   if (run_state.should_quit)
>     break;
>   // Try to do a unit of delayed work.
>   keep_running |= DoDelayedWork();
> } while (keep_running && !run_state.should_quit);
> 
> and have ::DoDelayedWork() return true if it actually ran a task?  then this
> loop would continue until nothing happened in Wait(true) and DoDelayedWork()

 That would be incorrect though. This must continue until we do not do anything
on Wait and the queue is empty. By the way, I just realized that the current
implementation is doing a busy loop. Let me try to refactor all of this.

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
File mojo/public/cpp/utility/tests/run_loop_unittest.cc (right):

https://chromiumcodereview.appspot.com/588593002/diff/40001/mojo/public/cpp/u...
mojo/public/cpp/utility/tests/run_loop_unittest.cc:450: ASSERT_LE(after-before,
5e4);
On 2014/09/22 22:30:18, jamesr wrote:
> this is super racy. if you want tests like this we need a way to inject a fake
> clock implementation.
> 
> why not just assert that we exited (i.e. that the test harness didn't time
out?)

 I didn't want to count on timeout because in case of error, it will delay the
CQ for a lot of time, but if this is considered OK, then yes, I can do it. It
makes the test a lot simpler.

[qsr, 2014-09-23 10:41:24]

On 2014/09/22 22:30:18, jamesr wrote:
> I don't understand the comment about RunUntilIdle().  If I post a task that
> attempts to quit and then invoke RunUntilIdle() it should hit the code you've
> added, right?

 It should hit the code, yes, but not in a way that would have fail before. My
change was so that if a quit is received on a task, then even if a handle is
ready at this point, it should not call the handler.

[jamesr, 2014-09-24 04:23:10]

https://chromiumcodereview.appspot.com/588593002/diff/60001/mojo/public/cpp/u...
File mojo/public/cpp/utility/lib/run_loop.cc (right):

https://chromiumcodereview.appspot.com/588593002/diff/60001/mojo/public/cpp/u...
mojo/public/cpp/utility/lib/run_loop.cc:115: if (!did_notify && until_idle &&
delayed_tasks_.empty())
RunUntilIdle does not mean run until the delayed task queue is empty, it means
run until the loop cannot make any more progress without sleeping.  Look at
base::MessageLoop for reference.

This isn't right.

[qsr, 2014-09-24 07:57:19]

> RunUntilIdle does not mean run until the delayed task queue is empty, it means
> run until the loop cannot make any more progress without sleeping.  Look at
> base::MessageLoop for reference.
> 
> This isn't right.


 Well, this is the current behavior. The loop of RunUntilIdle can currently quit
early only if delayed_tasks_.empty() is true.

 So, updated the code to have the same behavior as the base RunLoop. If someone
relies on the current behavior in a mojo application, this will break them.

[jamesr, 2014-09-24 08:04:32]

OK, that was a bug in RunLoop (almost certainly mine)

c++ changes lgtm but I don't understand the python changes - can you explain how
they are related?

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
File mojo/public/cpp/utility/lib/run_loop.cc (right):

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
mojo/public/cpp/utility/lib/run_loop.cc:103: void RunLoop::RunInternal(bool
until_idle) {
could we make this a 2-state enum instead of a bool? RunInternal(true) is a lot
harder to understand than RunInternal(UNTIL_IDLE)

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
File mojo/public/cpp/utility/tests/run_loop_unittest.cc (right):

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
mojo/public/cpp/utility/tests/run_loop_unittest.cc:417: QuittingTask(RunLoop*
run_loop) : run_loop(run_loop) {}
explicit

[qsr, 2014-09-24 08:58:49]

On 2014/09/24 08:04:32, jamesr wrote:
> OK, that was a bug in RunLoop (almost certainly mine)
> 
> c++ changes lgtm but I don't understand the python changes - can you explain
how
> they are related?

 The python test were relying on RunUntilIdle running until no task were on the
run loop. I also changed it to not depend on the clock anymore.

> 
>
https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
> File mojo/public/cpp/utility/lib/run_loop.cc (right):
> 
>
https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
> mojo/public/cpp/utility/lib/run_loop.cc:103: void RunLoop::RunInternal(bool
> until_idle) {
> could we make this a 2-state enum instead of a bool? RunInternal(true) is a
lot
> harder to understand than RunInternal(UNTIL_IDLE)
> 
>
https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
> File mojo/public/cpp/utility/tests/run_loop_unittest.cc (right):
> 
>
https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
> mojo/public/cpp/utility/tests/run_loop_unittest.cc:417: QuittingTask(RunLoop*
> run_loop) : run_loop(run_loop) {}
> explicit

[qsr, 2014-09-24 09:14:15]

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
File mojo/public/cpp/utility/lib/run_loop.cc (right):

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
mojo/public/cpp/utility/lib/run_loop.cc:103: void RunLoop::RunInternal(bool
until_idle) {
On 2014/09/24 08:04:32, jamesr wrote:
> could we make this a 2-state enum instead of a bool? RunInternal(true) is a
lot
> harder to understand than RunInternal(UNTIL_IDLE)

Done.

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
File mojo/public/cpp/utility/tests/run_loop_unittest.cc (right):

https://codereview.chromium.org/588593002/diff/100001/mojo/public/cpp/utility...
mojo/public/cpp/utility/tests/run_loop_unittest.cc:417: QuittingTask(RunLoop*
run_loop) : run_loop(run_loop) {}
On 2014/09/24 08:04:32, jamesr wrote:
> explicit

Done.

[qsr, 2014-09-24 09:14:18]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-24 09:15:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588593002/120001

[commit-bot: I haz the power, 2014-09-24 09:57:28]

Committed patchset #7 (id:120001) as 308c45b69a4f65374b5c602924e877b83412b228

[commit-bot: I haz the power, 2014-09-24 09:58:08]

Patchset 7 (id:??) landed as
https://crrev.com/8fd73aa30ad43651bb6dc425ef2156073e1a9377
Cr-Commit-Position: refs/heads/master@{#296378}