Join embeder threads on Windows.

Join embedder threads on Windows.

BUG=
R=sgjesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/9395a5b86f2f84c1a2d973b8c8b940ce369c957a

[zra, 2015-08-14 15:05:47]

zra@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-08-25 22:21:03]

Please ask Søren about the consolidation of the monitor and critical section.

Thanks,
-Ivan

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
File runtime/bin/dbg_connection_win.cc (right):

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
runtime/bin/dbg_connection_win.cc:24: MonitorLocker ml(handler_monitor_);
Assert that thread is not running and that the id is invalid.

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
runtime/bin/dbg_connection_win.cc:42: handler_thread_running_ = false;
Assert that thread is running beforehand and that id is valid.

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
runtime/bin/eventhandler_win.cc:128: read_thread_monitor_(new Monitor()) {
Can we share the monitor and the critical section?

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
runtime/bin/eventhandler_win.cc:196: read_thread_finished_ = true;
Assertions?

[zra, 2015-08-26 05:24:39]

zra@google.com changed reviewers:
+ sgjesse@google.com

[zra, 2015-08-26 05:24:39]

Soren, in the eventhandler implementation for windows, the class StdHandle uses
both a windows-style critical section, and a monitor. What is the reason for
using both? Could we use only a monitor, instead?

I ask because I am making a change in this CL to wait for the read threads
spawned by the Handle class to finish the same way the StdHandle class waits for
the writing threads to finish. I'm wondering if I need to use the same strategy
as StdHandle, or if using only a monitor is sufficient.

Thanks!

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
File runtime/bin/dbg_connection_win.cc (right):

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
runtime/bin/dbg_connection_win.cc:24: MonitorLocker ml(handler_monitor_);
On 2015/08/25 22:21:03, Ivan Posva wrote:
> Assert that thread is not running and that the id is invalid.

Done.

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
runtime/bin/dbg_connection_win.cc:42: handler_thread_running_ = false;
On 2015/08/25 22:21:03, Ivan Posva wrote:
> Assert that thread is running beforehand and that id is valid.

Done.

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
runtime/bin/eventhandler_win.cc:196: read_thread_finished_ = true;
On 2015/08/25 22:21:03, Ivan Posva wrote:
> Assertions?

Done.

[Søren Gjesse, 2015-08-26 07:28:36]

On 2015/08/26 05:24:39, zra wrote:
> Soren, in the eventhandler implementation for windows, the class StdHandle
uses
> both a windows-style critical section, and a monitor. What is the reason for
> using both? Could we use only a monitor, instead?
> 
> I ask because I am making a change in this CL to wait for the read threads
> spawned by the Handle class to finish the same way the StdHandle class waits
for
> the writing threads to finish. I'm wondering if I need to use the same
strategy
> as StdHandle, or if using only a monitor is sufficient.

I assume you are referring to the CRITICAL_SECTION declared in class Handle.
This
critical section is used to protect access to each Handle instance, and all the
methods in the superclass.

Changing the CRITICAL_SECTION to a Monitor, and changing ScopedLock to
MonitorLocker
should work exactly the same as Monitor is implemented using CRITICAL_SECTION.
Then
getting id of the additional Monitor in StdHandle should be possible by just
re-using
the now existing Monitor.

Looking at the code there might actually be a bug in StdHandle::RunWriteLoop
(and
its call to StdHandle::WriteSyncCompleteAsync). The Monitor is protecting the
additional fields added in StdHandle, but the CRITICAL_SECTION is still
protecting
fields from Handle, e.g. pending_write_. pending_write_ is accessed and mutated
in 
StdHandle::WriteSyncCompleteAsync, but only the Monitor not the CRITICAL_SECTION
is
held.

Having just one monitor here should avoid this.

> 
> Thanks!
> 
>
https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
> File runtime/bin/dbg_connection_win.cc (right):
> 
>
https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
> runtime/bin/dbg_connection_win.cc:24: MonitorLocker ml(handler_monitor_);
> On 2015/08/25 22:21:03, Ivan Posva wrote:
> > Assert that thread is not running and that the id is invalid.
> 
> Done.
> 
>
https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/dbg_connect...
> runtime/bin/dbg_connection_win.cc:42: handler_thread_running_ = false;
> On 2015/08/25 22:21:03, Ivan Posva wrote:
> > Assert that thread is running beforehand and that id is valid.
> 
> Done.
> 
>
https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
> File runtime/bin/eventhandler_win.cc (right):
> 
>
https://codereview.chromium.org/1291163002/diff/60001/runtime/bin/eventhandle...
> runtime/bin/eventhandler_win.cc:196: read_thread_finished_ = true;
> On 2015/08/25 22:21:03, Ivan Posva wrote:
> > Assertions?
> 
> Done.

[zra, 2015-08-27 08:12:00]

Changed Handle to use only a Monitor. PTAL.

[Søren Gjesse, 2015-08-27 13:28:24]

lgtm with comments

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:301: bool Handle::IssueRead() {
MonitorLocker ml(monitor_); here.

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:426: bool DirectoryWatchHandle::IssueRead() {
MonitorLocker ml(monitor_); here as well.

[zra, 2015-08-27 14:04:51]

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:301: bool Handle::IssueRead() {
On 2015/08/27 13:28:24, Søren Gjesse wrote:
> MonitorLocker ml(monitor_); here.

The monitor is already entered at calls to non-socket IssueRead()s.

https://codereview.chromium.org/1291163002/diff/100001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:426: bool DirectoryWatchHandle::IssueRead() {
On 2015/08/27 13:28:24, Søren Gjesse wrote:
> MonitorLocker ml(monitor_); here as well.

Ditto.

[zra, 2015-08-28 04:52:54]

Ivan, as we discussed this morning, I was able to move all blocking for the
reader thread to ReadComplete in the event handler thread. PTAL.

[zra, 2015-08-31 23:04:47]

On 2015/08/28 04:52:54, zra wrote:
> Ivan, as we discussed this morning, I was able to move all blocking for the
> reader thread to ReadComplete in the event handler thread. PTAL.

Ping

[Ivan Posva, 2015-09-01 21:15:03]

LGTMwC

-Ivan

https://codereview.chromium.org/1291163002/diff/120001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/1291163002/diff/120001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.h:267: Monitor* monitor_;
Usually we try to have the monitor/lock before the fields that are protected by
it.

[zra, 2015-09-02 00:13:16]

Thanks!

https://codereview.chromium.org/1291163002/diff/120001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/1291163002/diff/120001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.h:267: Monitor* monitor_;
On 2015/09/01 21:15:03, Ivan Posva wrote:
> Usually we try to have the monitor/lock before the fields that are protected
by
> it.

Done.

[zra, 2015-09-02 00:53:07]

Committed patchset #8 (id:140001) manually as
9395a5b86f2f84c1a2d973b8c8b940ce369c957a.