Simplify process exit-code handling on Posix, take 2.

Simplify process exit-code handling on Posix, take 2.

As in https://codereview.chromium.org/24024007, except the monitor->Wait
is now called in a loop with a guard, to ensure we actually do get notified.

Also, now correctly exiting the thread if we have no more child
processes, using the initialize/terminate mutex as guard.

BUG=
R=sgjesse@google.com

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

[Anders Johnsen, 2013-09-09 10:49:00]

Please take a look, I've changed how we handle monitor and simplified some of
the mutex logic. 

Thanks!,

- Anders

[Mads Ager (google), 2013-09-09 16:35:11]

A couple of comments. I can't convince myself that this does not have race
conditions. Have a look at these comments and we can chat about them tomorrow.

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:156: while (!initialized_) {
Shouldn't you be looping while initialized_ is true?

You are waiting for the thread to terminate. When the thread terminates it sets
initialized_ to false. So you have to loop while initialized_ is true. When it
becomes false there is no thread running and you have successfully waited for
termination?

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:198: terminate_ = true;
I guess setting terminate_ here doesn't really do anything because of the return
below. You could remove the return or remove the assignment to terminate_?

Actually, I think you have to remove the return here. You are terminating the
thread and therefore you should be signalling the thread_terminate_monitor_,
right?

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:205: initialized_ = false;
I don't like that we have collapsed thread_terminated_ and initialized_ into one
thing. The problem is that  initialized is now guarded by two different things:
the mutex and the thread terminate monitor. I can't convince myself that this
will always work correctly. I particular, here you are updating initialized and
you are guarding it by holding the thread_terminate_monitor. Some other thread
could be inspecting the value in EnsureInitialized which is guarded by the
mutex.

[Anders Johnsen, 2013-09-09 17:55:36]

PTAL. I've updated the linux version only, so let's keep the discussion in that
file.

I've done the following changes:
- Renamed the booleans. This makes it more clear what they are used for.
- Using correctly logic for the wait-while-condition.
- Added comments to explain why we don't have a race condition (given that we
agree the assumptions are correct).
- Use 'wait' instead of 'waitpid', as there now are no differences in the way we
use it.

Again, thanks for reviewing the code!

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
File runtime/bin/process_android.cc (right):

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:156: while (!initialized_) {
On 2013/09/09 16:35:11, Mads Ager (google) wrote:
> Shouldn't you be looping while initialized_ is true?
> 
> You are waiting for the thread to terminate. When the thread terminates it
sets
> initialized_ to false. So you have to loop while initialized_ is true. When it
> becomes false there is no thread running and you have successfully waited for
> termination?

You are absolutely right, I'll rename to better explain the usage.

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:198: terminate_ = true;
On 2013/09/09 16:35:11, Mads Ager (google) wrote:
> I guess setting terminate_ here doesn't really do anything because of the
return
> below. You could remove the return or remove the assignment to terminate_?
> 
> Actually, I think you have to remove the return here. You are terminating the
> thread and therefore you should be signalling the thread_terminate_monitor_,
> right?

As it's able to take the lock, it know that it's _not_ in the process of
terminating, but that we simply don't have any child processes. Now that it have
the lock, it can do a quick shutdown, by resetting the settings and returning
(stopping the thread).

If it doesn't get the lock, we are terminating. Simply continue, as the
terminating-setting will make sure we get down to the monitor notification.

That said, you are 100% right about setting terminate. Removed and overall
renamed the booleans to reflect this.

https://codereview.chromium.org/23717038/diff/4001/runtime/bin/process_androi...
runtime/bin/process_android.cc:205: initialized_ = false;
On 2013/09/09 16:35:11, Mads Ager (google) wrote:
> I don't like that we have collapsed thread_terminated_ and initialized_ into
one
> thing. The problem is that  initialized is now guarded by two different
things:
> the mutex and the thread terminate monitor. I can't convince myself that this
> will always work correctly. I particular, here you are updating initialized
and
> you are guarding it by holding the thread_terminate_monitor. Some other thread
> could be inspecting the value in EnsureInitialized which is guarded by the
> mutex.

Yes, the rename should have made this much clearer.

[Søren Gjesse, 2013-09-11 08:23:48]

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:114: // processes to die. That separate thread can
then get the exit code from
s/die/terminate

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:120: static bool EnsureInitialized() {
No need for a return value as it always returns true.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:128: // Start thread that handles process exits
when waitpid returns.
s/waitpid/wait

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:169: while (running_) {
We should not check running_ without holding the lock protecting it.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:216: static dart::Mutex* mutex_;
Can't we just get away with one monitor protecting both running_ and terminate_?

  while (true) {
    pid = TEMP_FAILURE_RETRY(wait(&status))
    monitor_->Enter();
    if (terminate_) {
      running_ = false;
      monitor->Notify();
      monitor_->Exit();
      return;
    }

    if (pid == -1) {
      running_ = false;
      monitor_->Exit();
      return;
    }

    // actual work.
    monitor_->Exit();
  }

[Anders Johnsen, 2013-09-11 11:54:52]

PTAL

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:114: // processes to die. That separate thread can
then get the exit code from
On 2013/09/11 08:23:48, Søren Gjesse wrote:
> s/die/terminate

Done.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:120: static bool EnsureInitialized() {
On 2013/09/11 08:23:48, Søren Gjesse wrote:
> No need for a return value as it always returns true.

Done.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:128: // Start thread that handles process exits
when waitpid returns.
On 2013/09/11 08:23:48, Søren Gjesse wrote:
> s/waitpid/wait

Done.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:169: while (running_) {
On 2013/09/11 08:23:48, Søren Gjesse wrote:
> We should not check running_ without holding the lock protecting it.

Done.

https://codereview.chromium.org/23717038/diff/9001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:216: static dart::Mutex* mutex_;
On 2013/09/11 08:23:48, Søren Gjesse wrote:
> Can't we just get away with one monitor protecting both running_ and
terminate_?
> 
>   while (true) {
>     pid = TEMP_FAILURE_RETRY(wait(&status))
>     monitor_->Enter();
>     if (terminate_) {
>       running_ = false;
>       monitor->Notify();
>       monitor_->Exit();
>       return;
>     }
> 
>     if (pid == -1) {
>       running_ = false;
>       monitor_->Exit();
>       return;
>     }
> 
>     // actual work.
>     monitor_->Exit();
>   }

Rewrote to only have one monitor in total.

[Søren Gjesse, 2013-09-11 12:10:12]

LGTM - with the same changes to Android and Mac OS

https://codereview.chromium.org/23717038/diff/14001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/23717038/diff/14001/runtime/bin/process_linux...
runtime/bin/process_linux.cc:121: static void Increment() {
Maybe call this "ProcessStarted".

[Anders Johnsen, 2013-09-11 13:05:02]

Thanks, landing.

https://codereview.chromium.org/23717038/diff/14001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/23717038/diff/14001/runtime/bin/process_linux...
runtime/bin/process_linux.cc:121: static void Increment() {
On 2013/09/11 12:10:12, Søren Gjesse wrote:
> Maybe call this "ProcessStarted".

Done.

[Anders Johnsen, 2013-09-11 13:07:04]

Committed patchset #5 manually as r27380 (presubmit successful).