diff options
author | Brendan Zagaeski <brendan.zagaeski@xamarin.com> | 2017-11-08 18:02:37 +0300 |
---|---|---|
committer | Ludovic Henry <ludovic@xamarin.com> | 2017-11-08 18:12:33 +0300 |
commit | 40be6362c67068c406065103292cbc8dee236ecc (patch) | |
tree | c4e078cf46188770dd7532ef11fcf0b202e2ab4d /mcs | |
parent | ea060b586dfbe16748a8e7b2da2d5990426d61fb (diff) |
[System] Add missing ConfigureAwait (#5963)
This is a candidate fix for:
https://bugzilla.xamarin.com/show_bug.cgi?id=60317
The bug was introduced by 1b5e0f73316ee7b83c8947bc738015443fabd7af.
Preliminary verification
========================
If I make just this _one_ change to a "bad" commit of Mono from after
commit 1b5e0f7, rebuild System.dll in the `xammac` profile, and then
overwrite that file in the MonoBundle directory of a Xamarin.Mac test
app, then I restore the old "good" behavior. (I started with
38da0b3b4996357a7472e3202c575c9111469721 as the "bad" version for my
tests.)
I believe this confirms that the cause of bug 60317 was the usual
deadlocking issue when awaiting Tasks on the UI thread. As discussed on
https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html, if
any Tasks awaited anywhere up the call stack are missing
`ConfigureAwait (false)`, then you can run into trouble on the UI
thread.
Additional little checks
========================
1. Do the other `await` statements in commit
1b5e0f73316ee7b83c8947bc738015443fabd7af all look OK? Yes, it looks
like this was the only `await` statement that was missing
`ConfigureAwait (false)`:
```
$ git show 1b5e0f73316ee7b83c8947bc738015443fabd7af | grep await
+ await ProcessOperation (cancellationToken).ConfigureAwait (false);
+ var ret = await InnerRead (cancellationToken).ConfigureAwait (false);
+ await Parent.InnerWrite (RunSynchronously, cancellationToken);
+ var ret = await Parent.InnerRead (RunSynchronously, requestedSize, cancellationToken).ConfigureAwait (false);
+ result = await asyncRequest.StartOperation (CancellationToken.None).ConfigureAwait (false);
+ result = await asyncRequest.StartOperation (cancellationToken).ConfigureAwait (false);
+ var ret = await task.ConfigureAwait (false);
+ await task.ConfigureAwait (false);
```
2. Is it a concern that mcs/class/System did not contain any calls to
`ConfigureAwait ()` until commit 1b5e0f7:
```
$ git grep ConfigureAwait 1b5e0f73316ee7b83c8947bc738015443fabd7af^ -- mcs/class/System || echo 'None'
None
```
I do not think this is a concern. For example,
System.Net.Http.HttpClient has been using `ConfigureAwait (false)` for
several years, so there is a precedent for it.
Diffstat (limited to 'mcs')
-rw-r--r-- | mcs/class/System/Mono.Net.Security/AsyncProtocolRequest.cs | 2 |
1 files changed, 1 insertions, 1 deletions
diff --git a/mcs/class/System/Mono.Net.Security/AsyncProtocolRequest.cs b/mcs/class/System/Mono.Net.Security/AsyncProtocolRequest.cs index 8ed25908b0f..3406cae2c0f 100644 --- a/mcs/class/System/Mono.Net.Security/AsyncProtocolRequest.cs +++ b/mcs/class/System/Mono.Net.Security/AsyncProtocolRequest.cs @@ -227,7 +227,7 @@ namespace Mono.Net.Security if (Interlocked.Exchange (ref WriteRequested, 0) != 0) { // Flush the write queue. Debug ("ProcessOperation - flushing write queue"); - await Parent.InnerWrite (RunSynchronously, cancellationToken); + await Parent.InnerWrite (RunSynchronously, cancellationToken).ConfigureAwait (false); } Debug ("ProcessOperation done: {0} -> {1}", status, newStatus); |