Fix for issue 28606 - Wait for thread that runs the interrupted isolate to exit completely before e…

Fix for issue 28606 - Wait for thread that runs the interrupted isolate to exit completely before exiting the main thread, we get into racy cleanup otherwise.

R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/3f48197f370a393ae57811c7db51dda061d21d54

[siva, 2017-02-04 02:33:49]

asiva@google.com changed reviewers:
+ fschneider@google.com

[hausner, 2017-02-04 06:18:37]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-02-04 06:18:37]

DBC

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:1590: EXPECT(interrupt_isolate_id ==
ILLEGAL_ISOLATE_ID);
I guess we could eliminate waiting for the isolate to be shutdown and move this
EXPECT after the wait loop below (after isolate has been shut down).

[siva, 2017-02-06 20:15:16]

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:1590: EXPECT(interrupt_isolate_id ==
ILLEGAL_ISOLATE_ID);
On 2017/02/04 06:18:37, hausner wrote:
> I guess we could eliminate waiting for the isolate to be shutdown and move
this
> EXPECT after the wait loop below (after isolate has been shut down).

The test wants to test the three stages that the handler reacts to (start of an
isolate, interrupting an isolate while it is running, stopping an isolate). So
we should retain the shutdown part as it tests the shutdown handler callback
mechanism.

[Florian Schneider, 2017-02-06 21:02:28]

Lgtm.

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:1553: interrupt_thread_stopped = false;
Could the other thread see an uninitialized value here? To be safe I'd also wrap
this access with the locker:

{
  MonitorLocker ml(sync);
  interrupt_thread_stopped = false;
}

[siva, 2017-02-06 21:17:41]

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/2671133002/diff/40001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:1553: interrupt_thread_stopped = false;
On 2017/02/06 21:02:28, Florian Schneider wrote:
> Could the other thread see an uninitialized value here? To be safe I'd also
wrap
> this access with the locker:
> 
> {
>   MonitorLocker ml(sync);
>   interrupt_thread_stopped = false;
> }

The other thread starts after this assignment so should be ok, but I have added
the lock to be safe and ensure there are no memory barrier issues.

[siva, 2017-02-07 00:26:01]

Description was changed from

==========
Fix for issue 28606 - Wait for thread that runs the interrupted isolate to exit
completely before exiting the main thread, we get into racy cleanup otherwise.
==========

to

==========
Fix for issue 28606 - Wait for thread that runs the interrupted isolate to exit
completely before exiting the main thread, we get into racy cleanup otherwise.

R=fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3f48197f370a393ae57811c7db51dda061d21d54
==========

[siva, 2017-02-07 00:26:02]

Committed patchset #4 (id:60001) manually as
3f48197f370a393ae57811c7db51dda061d21d54 (presubmit successful).