From fcc7e6f7636c2de21eed89f9da1a82207c6055a6 Mon Sep 17 00:00:00 2001 From: monojenkins Date: Mon, 25 Jun 2018 17:17:13 -0400 Subject: [PATCH] [2018-06] [System]: `HttpWebRequest` now throws `WebExceptionStatus.RequestCanceled` on abort. (#9294) * [System]`HttpWebRequest` now throws `WebExceptionStatus.RequestCanceled` on abort. #9031. Cleanup the internal `HttpWebRequest.RunWithTimeout()` function and use `GetWebException()` everywhere to throw `WebException` with `WebExceptionStatus.RequestCanceled` on abort. New tests for this will be added to the web-tests by extending the existing `Xamarin.WebTests.HttpInstrumentationTests.AbortDuringHandshake` to check the synchronous and begin/end async APIs as well, using both `GET` and `POST`. See https://github.com/xamarin/web-tests/blob/master/Xamarin.WebTests.Tests/Xamarin.WebTests.HttpInstrumentationTests/AbortDuringHandshake.cs. Fixes #9031. * Fix exception checks, setting `TransferEncoding` without `SendChunked` is supposed to throw `InvalidOperationException`. * Fix the incorrect behavior in this test. Backport of #9212. --- .../Test/System.Net.Http/HttpClientTest.cs | 2 +- mcs/class/System/System.Net/HttpWebRequest.cs | 79 +++++++++++----------- mcs/class/System/System.Net/WebResponseStream.cs | 2 +- 3 files changed, 40 insertions(+), 43 deletions(-) diff --git a/mcs/class/System.Net.Http/Test/System.Net.Http/HttpClientTest.cs b/mcs/class/System.Net.Http/Test/System.Net.Http/HttpClientTest.cs index 62e260b37e9..63cddcda8f6 100644 --- a/mcs/class/System.Net.Http/Test/System.Net.Http/HttpClientTest.cs +++ b/mcs/class/System.Net.Http/Test/System.Net.Http/HttpClientTest.cs @@ -769,7 +769,7 @@ namespace MonoTests.System.Net.Http client.SendAsync (request, HttpCompletionOption.ResponseHeadersRead).Wait (); Assert.Fail ("#1"); } catch (AggregateException e) { - Assert.AreEqual (typeof (ProtocolViolationException), e.InnerException.GetType (), "#2"); + Assert.AreEqual (typeof (InvalidOperationException), e.InnerException.GetType (), "#2"); } Assert.IsNull (failed, "#102"); } finally { diff --git a/mcs/class/System/System.Net/HttpWebRequest.cs b/mcs/class/System/System.Net/HttpWebRequest.cs index bda416f2536..74dd1597620 100644 --- a/mcs/class/System/System.Net/HttpWebRequest.cs +++ b/mcs/class/System/System.Net/HttpWebRequest.cs @@ -434,7 +434,6 @@ namespace System.Net } public string Host { - get { Uri uri = hostUri ?? Address; return (hostUri == null || !hostHasPort) && Address.IsDefaultPort ? @@ -858,22 +857,21 @@ namespace System.Net if (Aborted) throw CreateRequestAbortedException (); - bool send = !(method == "GET" || method == "CONNECT" || method == "HEAD" || - method == "TRACE"); + bool send = !(method == "GET" || method == "CONNECT" || method == "HEAD" || method == "TRACE"); if (method == null || !send) - throw new ProtocolViolationException ("Cannot send data when method is: " + method); + throw new ProtocolViolationException (SR.net_nouploadonget); if (contentLength == -1 && !sendChunked && !allowBuffering && KeepAlive) throw new ProtocolViolationException ("Content-Length not set"); string transferEncoding = TransferEncoding; if (!sendChunked && transferEncoding != null && transferEncoding.Trim () != "") - throw new ProtocolViolationException ("SendChunked should be true."); + throw new InvalidOperationException (SR.net_needchunked); WebOperation operation; lock (locker) { if (getResponseCalled) - throw new InvalidOperationException ("The operation cannot be performed once the request has been submitted."); + throw new InvalidOperationException (SR.net_reqsubmitted); operation = currentOperation; if (operation == null) { @@ -900,7 +898,7 @@ namespace System.Net try { return TaskToApm.End (asyncResult); } catch (Exception e) { - throw FlattenException (e); + throw GetWebException (e); } } @@ -909,7 +907,7 @@ namespace System.Net try { return GetRequestStreamAsync ().Result; } catch (Exception e) { - throw FlattenException (e); + throw GetWebException (e); } } @@ -925,35 +923,18 @@ namespace System.Net } internal static Task RunWithTimeout ( - Func> func, int timeout, Action abort) - { - // Call `func` here to propagate any potential exception that it - // might throw to our caller rather than returning a faulted task. - var cts = new CancellationTokenSource (); - var workerTask = func (cts.Token); - return RunWithTimeoutWorker (workerTask, timeout, abort, cts); - } - - internal static Task RunWithTimeout ( Func> func, int timeout, Action abort, - CancellationToken cancellationToken) + Func aborted, CancellationToken cancellationToken) { var cts = CancellationTokenSource.CreateLinkedTokenSource (cancellationToken); - return RunWithTimeout (func, timeout, abort, cts); - } - - static Task RunWithTimeout ( - Func> func, int timeout, Action abort, - CancellationTokenSource cts) - { // Call `func` here to propagate any potential exception that it // might throw to our caller rather than returning a faulted task. var workerTask = func (cts.Token); - return RunWithTimeoutWorker (workerTask, timeout, abort, cts); + return RunWithTimeoutWorker (workerTask, timeout, abort, aborted, cts); } static async Task RunWithTimeoutWorker ( - Task workerTask, int timeout, Action abort, + Task workerTask, int timeout, Action abort, Func aborted, CancellationTokenSource cts) { try { @@ -967,7 +948,7 @@ namespace System.Net } throw new WebException (SR.net_timeout, WebExceptionStatus.Timeout); } catch (Exception ex) { - throw FlattenException (ex); + throw GetWebException (ex, aborted ()); } finally { cts.Dispose (); } @@ -975,7 +956,11 @@ namespace System.Net Task RunWithTimeout (Func> func) { - return RunWithTimeout (func, timeout, Abort); + // Call `func` here to propagate any potential exception that it + // might throw to our caller rather than returning a faulted task. + var cts = new CancellationTokenSource (); + var workerTask = func (cts.Token); + return RunWithTimeoutWorker (workerTask, timeout, Abort, () => Aborted, cts); } async Task MyGetResponseAsync (CancellationToken cancellationToken) @@ -983,13 +968,6 @@ namespace System.Net if (Aborted) throw CreateRequestAbortedException (); - if (method == null) - throw new ProtocolViolationException ("Method is null."); - - string transferEncoding = TransferEncoding; - if (!sendChunked && transferEncoding != null && transferEncoding.Trim () != "") - throw new ProtocolViolationException ("SendChunked should be true."); - var completion = new WebCompletionSource (); WebOperation operation; lock (locker) { @@ -1164,12 +1142,17 @@ namespace System.Net WebException GetWebException (Exception e) { + return GetWebException (e, Aborted); + } + + static WebException GetWebException (Exception e, bool aborted) + { e = FlattenException (e); if (e is WebException wexc) { - if (!Aborted || wexc.Status == WebExceptionStatus.RequestCanceled || wexc.Status == WebExceptionStatus.Timeout) + if (!aborted || wexc.Status == WebExceptionStatus.RequestCanceled || wexc.Status == WebExceptionStatus.Timeout) return wexc; } - if (Aborted || e is OperationCanceledException || e is ObjectDisposedException) + if (aborted || e is OperationCanceledException || e is ObjectDisposedException) return CreateRequestAbortedException (); return new WebException (e.Message, e, WebExceptionStatus.UnknownError, null); } @@ -1184,6 +1167,20 @@ namespace System.Net if (Aborted) throw CreateRequestAbortedException (); + string transferEncoding = TransferEncoding; + if (!sendChunked && transferEncoding != null && transferEncoding.Trim () != "") { + /* + * The only way we could get here without already catching this in the + * `TransferEncoding` property settor is via HttpClient, which does not + * do strict checking on all headers. + * + * We can remove this check again after switching to the CoreFX version + * of HttpClient. + * + */ + throw new InvalidOperationException (SR.net_needchunked); + } + return TaskToApm.Begin (RunWithTimeout (MyGetResponseAsync), callback, state); } @@ -1195,7 +1192,7 @@ namespace System.Net try { return TaskToApm.End (asyncResult); } catch (Exception e) { - throw FlattenException (e); + throw GetWebException (e); } } @@ -1213,7 +1210,7 @@ namespace System.Net try { return GetResponseAsync ().Result; } catch (Exception e) { - throw FlattenException (e); + throw GetWebException (e); } } diff --git a/mcs/class/System/System.Net/WebResponseStream.cs b/mcs/class/System/System.Net/WebResponseStream.cs index d7d3e978985..bad3604cd5f 100644 --- a/mcs/class/System/System.Net/WebResponseStream.cs +++ b/mcs/class/System/System.Net/WebResponseStream.cs @@ -181,7 +181,7 @@ namespace System.Net () => { Operation.Abort (); innerStream.Dispose (); - }, cancellationToken); + }, () => Operation.Aborted, cancellationToken); } bool CheckAuthHeader (string headerName) -- 2.11.4.GIT