Fix infrequent hangs in test-runner. (#16793)
commit7fa706a316f043068e4d37a7d512bd44295e14b0
authorJohan Lorensson <lateralusx.github@gmail.com>
Mon, 16 Sep 2019 00:51:07 +0000 (16 02:51 +0200)
committerLarry Ewing <lewing@microsoft.com>
Mon, 16 Sep 2019 00:51:07 +0000 (15 19:51 -0500)
tree390d55c2d2944d45b15d6b4ac67c838ebb7f0b24
parent8da3616e7fe0ad17d249f8f00df96d3e6c683729
Fix infrequent hangs in test-runner. (#16793)

* Fix infrequent hangs in test-runner.

test-runner has been plagued with very infrequent hangs causing long
timeouts on CI. Turns out that there is a race in AsyncStreamReader
used by Process class for handling stdout/stderr when closing down
process. Since AsyncStreamReader::Dispose didn't make sure no pending
async read requests were still in flight before closing the stream
and underlying handle, it led to a race condition deadlocking the
complete process, hanging test-runner. A similar race has also been
observed but instead of causing a deadlock, it could also manifest as an
System.ObjectDisposedException when doing EndRead while another thread
is disposing the underlying stream. Hitting that race didn't deadlock
the process but failed to complete invoke of test-runner.

Fix is to better protect the async callback and the thread doing the
dispose call. In order to make sure we don't close underlying handle
while still having async reads in flight, fix also checks async result
for completion and if not completed when trying to dispose AsyncStreamReader
it will try to cancel outstanding pending IO and wait for completion
before closing the stream.

Since the issue has only been seen on Windows, the fix will try
to cancel pending IO and wait for completion before closing stream
on Windows. On other platforms, old behavior will be preserved, meaning
that the implementation will still be vunarable to race between pending
reads and close of stream, but since problem has not been observed on
other platforms and since there is no good way of cancel outstanding
blocking reads, old behavior is kept.

At some point in future we should look into replacing the implementation
of AsyncStreamReader and use cancelation tokens, but since we don't have
all corefx support around cancelation in Mono BCL, that currently won't
solve the issue, meaning that we need to bring in more classes. Since the
problem also occur in test-runner using the build BCL profile, even more
needs to be added to that profile in order to get similar support. But due
to the infrequency of the problem and that it has only been observed on
Windows, the isolated fix seems more reasonable.

* Build error fix.

* Review feedback.

* Fix type cast not working on linux builds.

* Adjust icall naming in icall-def.h.
mcs/class/corlib/System.IO/MonoIO.cs
mcs/class/corlib/System.IO/MonoIOError.cs
mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs
mono/metadata/icall-def.h
mono/metadata/w32file-unix.c
mono/metadata/w32file-win32.c
mono/metadata/w32file.c
mono/metadata/w32file.h