Skip to content

Commit 5f9a2db

Browse files
monojenkinsakoeplinger
monojenkins
authored and
akoeplinger
committed
[2019-08] Fix infrequent hangs in test-runner. (#16854)
* 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. Backport of #16793.
1 parent f31f5ea commit 5f9a2db

File tree

8 files changed

+100
-17
lines changed

8 files changed

+100
-17
lines changed

‎mcs/class/corlib/System.IO/MonoIO.cs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,21 @@ public static IntPtr Open (string filename,
433433
}
434434
}
435435

436+
[MethodImplAttribute (MethodImplOptions.InternalCall)]
437+
private extern static bool Cancel_internal (IntPtr handle, out MonoIOError error);
438+
439+
internal static bool Cancel (SafeHandle safeHandle, out MonoIOError error)
440+
{
441+
bool release = false;
442+
try {
443+
safeHandle.DangerousAddRef (ref release);
444+
return Cancel_internal (safeHandle.DangerousGetHandle (), out error);
445+
} finally {
446+
if (release)
447+
safeHandle.DangerousRelease ();
448+
}
449+
}
450+
436451
[MethodImplAttribute (MethodImplOptions.InternalCall)]
437452
public extern static bool Close (IntPtr handle,
438453
out MonoIOError error);

‎mcs/class/corlib/System.IO/MonoIOError.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ internal enum MonoIOError: int {
7575
ERROR_SHARING_BUFFER_EXCEEDED = 36,
7676
ERROR_HANDLE_EOF = 38,
7777
*/ ERROR_HANDLE_DISK_FULL = 39,
78-
/* ERROR_NOT_SUPPORTED = 50,
79-
ERROR_REM_NOT_LIST = 51,
78+
ERROR_NOT_SUPPORTED = 50,
79+
/* ERROR_REM_NOT_LIST = 51,
8080
ERROR_DUP_NAME = 52,
8181
ERROR_BAD_NETPATH = 53,
8282
ERROR_NETWORK_BUSY = 54,

‎mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs‎

Lines changed: 47 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@ namespace System.Diagnostics {
1919
using System.IO;
2020
using System.Text;
2121
using System.Runtime.InteropServices;
22-
using System.Threading;
23-
using System.Collections;
22+
using System.Threading;
23+
using System.Collections;
24+
using Microsoft.Win32.SafeHandles;
2425

2526
internal delegate void UserCallBack(String data);
2627

@@ -58,10 +59,10 @@ internal class AsyncStreamReader : IDisposable
5859

5960
// Cache the last position scanned in sb when searching for lines.
6061
private int currentLinePos;
61-
6262
#if MONO
63-
//users to coordinate between Dispose and BeginReadLine
64-
private object syncObject = new Object ();
63+
//users to coordinate between Dispose and ReadBuffer
64+
private object syncObject = new Object ();
65+
private IAsyncResult asyncReadResult = null;
6566
#endif
6667

6768
internal AsyncStreamReader(Process process, Stream stream, UserCallBack callback, Encoding encoding)
@@ -115,8 +116,31 @@ protected virtual void Dispose(bool disposing)
115116
lock (syncObject) {
116117
#endif
117118
if (disposing) {
118-
if (stream != null)
119+
if (stream != null) {
120+
#if MONO
121+
if (asyncReadResult != null && !asyncReadResult.IsCompleted) {
122+
// Closing underlying stream when having pending async read request in progress is
123+
// not portable and racy by design. Try to cancel pending async read before closing stream.
124+
// We are still holding lock that will prevent new async read requests to queue up
125+
// before we have closed and invalidated the stream.
126+
if (stream is FileStream) {
127+
SafeHandle tmpStreamHandle = ((FileStream)stream).SafeFileHandle;
128+
while (!asyncReadResult.IsCompleted) {
129+
MonoIOError error;
130+
if (!MonoIO.Cancel (tmpStreamHandle, out error) && error == MonoIOError.ERROR_NOT_SUPPORTED) {
131+
// Platform don't support canceling pending IO requests on stream. If an async pending read
132+
// is still in flight when closing stream, it could trigger a race condition.
133+
break;
134+
}
135+
136+
// Wait for a short time for pending async read to cancel/complete/fail.
137+
asyncReadResult.AsyncWaitHandle.WaitOne (200);
138+
}
139+
}
140+
}
141+
#endif
119142
stream.Close();
143+
}
120144
}
121145
if (stream != null) {
122146
stream = null;
@@ -151,7 +175,11 @@ internal void BeginReadLine() {
151175

152176
if( sb == null ) {
153177
sb = new StringBuilder(DefaultBufferSize);
178+
#if MONO
179+
asyncReadResult = stream.BeginRead (byteBuffer, 0 , byteBuffer.Length, new AsyncCallback (ReadBuffer), null);
180+
#else
154181
stream.BeginRead(byteBuffer, 0 , byteBuffer.Length, new AsyncCallback(ReadBuffer), null);
182+
#endif
155183
}
156184
else {
157185
FlushMessageQueue();
@@ -169,12 +197,17 @@ private void ReadBuffer(IAsyncResult ar) {
169197

170198
try {
171199
#if MONO
172-
var stream = this.stream;
173-
if (stream == null)
174-
byteLen = 0;
175-
else
176-
#endif
200+
lock (syncObject) {
201+
Debug.Assert (ar.IsCompleted);
202+
asyncReadResult = null;
203+
if (this.stream == null)
204+
byteLen = 0;
205+
else
206+
byteLen = stream.EndRead (ar);
207+
}
208+
#else
177209
byteLen = stream.EndRead(ar);
210+
#endif
178211
}
179212
catch (IOException ) {
180213
// We should ideally consume errors from operations getting cancelled
@@ -242,10 +275,10 @@ private void ReadBuffer(IAsyncResult ar) {
242275
byteLen = 0;
243276
goto retry_dispose;
244277
}
245-
#endif
246-
stream.BeginRead(byteBuffer, 0 , byteBuffer.Length, new AsyncCallback(ReadBuffer), null);
247-
#if MONO
278+
asyncReadResult = stream.BeginRead (byteBuffer, 0 , byteBuffer.Length, new AsyncCallback(ReadBuffer), null);
248279
}
280+
#else
281+
stream.BeginRead(byteBuffer, 0 , byteBuffer.Length, new AsyncCallback(ReadBuffer), null);
249282
#endif
250283
}
251284
}

‎mono/metadata/icall-def.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,8 @@ HANDLES(MMAPIMPL_5, "OpenFileInternal", mono_mmap_open_file, gpointer, 9, (const
407407
HANDLES(MMAPIMPL_6, "OpenHandleInternal", mono_mmap_open_handle, gpointer, 7, (gpointer, const_gunichar2_ptr, int, gint64_ref, int, int, int_ref))
408408
HANDLES(MMAPIMPL_7, "Unmap", mono_mmap_unmap, MonoBoolean, 1, (gpointer))
409409

410-
ICALL_TYPE(MONOIO, "System.IO.MonoIO", MONOIO_1)
410+
ICALL_TYPE(MONOIO, "System.IO.MonoIO", MONOIO_39)
411+
NOHANDLES(ICALL(MONOIO_39, "Cancel_internal", ves_icall_System_IO_MonoIO_Cancel))
411412
NOHANDLES(ICALL(MONOIO_1, "Close(intptr,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_Close))
412413
#ifndef PLATFORM_RO_FS
413414
NOHANDLES(ICALL(MONOIO_2, "CopyFile(char*,char*,bool,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_CopyFile))

‎mono/metadata/w32file-unix.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2072,6 +2072,13 @@ mono_w32file_create(const gunichar2 *name, guint32 fileaccess, guint32 sharemode
20722072
return GINT_TO_POINTER(((MonoFDHandle*) filehandle)->fd);
20732073
}
20742074

2075+
gboolean
2076+
mono_w32file_cancel (gpointer handle)
2077+
{
2078+
mono_w32error_set_last (ERROR_NOT_SUPPORTED);
2079+
return FALSE;
2080+
}
2081+
20752082
gboolean
20762083
mono_w32file_close (gpointer handle)
20772084
{

‎mono/metadata/w32file-win32.c‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ mono_w32file_create(const gunichar2 *name, guint32 fileaccess, guint32 sharemode
6363
return res;
6464
}
6565

66+
gboolean
67+
mono_w32file_cancel (gpointer handle)
68+
{
69+
return CancelIoEx (handle, NULL);
70+
}
71+
6672
gboolean
6773
mono_w32file_close (gpointer handle)
6874
{

‎mono/metadata/w32file.c‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,19 @@ ves_icall_System_IO_MonoIO_Close (HANDLE handle, gint32 *error)
506506
return(ret);
507507
}
508508

509+
MonoBoolean
510+
ves_icall_System_IO_MonoIO_Cancel (HANDLE handle, gint32 *error)
511+
{
512+
gboolean ret;
513+
*error=ERROR_SUCCESS;
514+
515+
ret = mono_w32file_cancel (handle);
516+
if (ret == FALSE)
517+
*error = mono_w32error_get_last ();
518+
519+
return ret;
520+
}
521+
509522
gint32
510523
ves_icall_System_IO_MonoIO_Read (HANDLE handle, MonoArrayHandle dest,
511524
gint32 dest_offset, gint32 count,

‎mono/metadata/w32file.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,11 @@ gpointer
164164
ves_icall_System_IO_MonoIO_Open (const gunichar2 *filename, gint32 mode,
165165
gint32 access_mode, gint32 share, gint32 options,
166166
gint32 *error);
167+
168+
ICALL_EXPORT
169+
MonoBoolean
170+
ves_icall_System_IO_MonoIO_Cancel (gpointer handle, gint32 *error);
171+
167172
ICALL_EXPORT
168173
MonoBoolean
169174
ves_icall_System_IO_MonoIO_Close (gpointer handle, gint32 *error);
@@ -369,6 +374,9 @@ mono_w32file_cleanup (void);
369374
gpointer
370375
mono_w32file_create(const gunichar2 *name, guint32 fileaccess, guint32 sharemode, guint32 createmode, guint32 attrs);
371376

377+
gboolean
378+
mono_w32file_cancel (gpointer handle);
379+
372380
gboolean
373381
mono_w32file_close (gpointer handle);
374382

0 commit comments

Comments
 (0)