Make service protocol respect isolate lifecycle

Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Closes https://github.com/dart-lang/sdk/issues/28624

R=asiva@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/5534568ff1f3d598b61c40f46a477e32a39bdea0

[cbernaschina, 2017-08-30 22:46:35]

Description was changed from

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoidind the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Related https://github.com/dart-lang/sdk/issues/28624
==========

to

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Related https://github.com/dart-lang/sdk/issues/28624
==========

[cbernaschina, 2017-08-30 22:46:50]

cbernaschina@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[rmacnak, 2017-08-30 23:31:52]

lgtm

https://codereview.chromium.org/3006883002/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/3006883002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2004: // Isolate is runnable but not paused on start.
// Some service clients get confused if they see NotRunnable -> Runnable ->
PausedAtStart. Treat Runnable+ShouldPauseOnStart as NotRunnable so they see
NonRunnable -> PausedAtStart.

[cbernaschina, 2017-08-30 23:53:57]

https://codereview.chromium.org/3006883002/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/3006883002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2004: // Isolate is runnable but not paused on start.
On 2017/08/30 23:31:52, rmacnak wrote:
> // Some service clients get confused if they see NotRunnable -> Runnable ->
> PausedAtStart. Treat Runnable+ShouldPauseOnStart as NotRunnable so they see
> NonRunnable -> PausedAtStart.

Done.

[siva, 2017-08-30 23:56:30]

lgtm

https://codereview.chromium.org/3006883002/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/issue_30555_test.dart (right):

https://codereview.chromium.org/3006883002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/issue_30555_test.dart:122: "to do it.");
Maybe we should flip these two expects, first check if the ID matches up and
then the kind.

https://codereview.chromium.org/3006883002/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/3006883002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1999: if (message_handler()->is_paused_on_start()) {
Access to should_pause_on_start and is_paused_on_start still seem racy but I
guess you made sure the state shown is an earlier state which is safer than
showing a newer state.

[cbernaschina, 2017-08-31 00:04:45]

https://codereview.chromium.org/3006883002/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/issue_30555_test.dart (right):

https://codereview.chromium.org/3006883002/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/issue_30555_test.dart:122: "to do it.");
On 2017/08/30 23:56:30, siva wrote:
> Maybe we should flip these two expects, first check if the ID matches up and
> then the kind.

Done.

[cbernaschina, 2017-08-31 00:07:42]

Description was changed from

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Related https://github.com/dart-lang/sdk/issues/28624
==========

to

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Closes https://github.com/dart-lang/sdk/issues/28624
==========

[cbernaschina, 2017-08-31 00:09:42]

Description was changed from

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Closes https://github.com/dart-lang/sdk/issues/28624
==========

to

==========
Make service protocol respect isolate lifecycle

Status:
- Currently the getIsolate API tries to guess the status of the isolate
  when it is in between the IsolateRunnable and PauseStart, by reporting
  it as already in the PauseStart status.
  This can potentially make flaky all the tests that uses PauseStart to
  synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
  potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
  to a getIsolate responde with a PauseStart status to be sent before
  the actual PauseStart event.

Changes:
- Fallback to None pause event if the isolate is in between
  IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
  avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
  avoiding the race condition.

Closes https://github.com/dart-lang/sdk/issues/28624

R=asiva@google.com, rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/5534568ff1f3d598b61c40f46a477e32a39bdea0
==========

[cbernaschina, 2017-08-31 00:09:44]

Committed patchset #4 (id:60001) manually as
5534568ff1f3d598b61c40f46a477e32a39bdea0 (presubmit successful).