From 9988a9df35020a0ebccb1cdbe33906ee63432853 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Wed, 1 Mar 2017 11:54:38 +0100 Subject: [PATCH 1/6] [Foundation] Set 'sentRequest' when sending a request in NSUrlSessionHandler. Fixes this compiler warning: > [..]/external/mono/mcs/class/System.Net.Http/HttpClientEx.cs(50,8): warning CS0649: Field `Foundation.NSUrlSessionHandler.sentRequest' is never assigned to, and will always have its default value `false' However it changes the runtime behavior, and we'll now throw an exception in cases that we accepted before: * `sentRequest` is only read in `EnsureModifiability ()`, which throws an exception if `sentRequest` is true. * Previously `sentRequest` was never set (thus the compiler warning), which meant `EnsureModifiability` would never throw an exception. * Looking at the similar `CFNetworkHandler` (which has the identical field and methods), it seems that the intended behavior is to set `sentRequest` in `SendAsync`, and then `EnsureModifiability` is called whenever a property is set to ensure the property isn't set too late (and any change would be ignored because the request was already sent). * This means that previously setting any property after the request was sent would not throw any exceptions (even though the change would be ignored), while with this change we'd start throwing exceptions. --- src/Foundation/NSUrlSessionHandler.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Foundation/NSUrlSessionHandler.cs b/src/Foundation/NSUrlSessionHandler.cs index 3d348d71decf..8bb1aca35a9a 100644 --- a/src/Foundation/NSUrlSessionHandler.cs +++ b/src/Foundation/NSUrlSessionHandler.cs @@ -169,6 +169,8 @@ async Task CreateRequest (HttpRequestMessage request) #endif protected override async Task SendAsync (HttpRequestMessage request, CancellationToken cancellationToken) { + sentRequest = true; + var nsrequest = await CreateRequest (request).ConfigureAwait(false); var dataTask = session.CreateDataTask (nsrequest); From 7e6f0469184cc3a9cf6cab65da2496849624cddf Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Date: Thu, 18 May 2017 16:08:30 +0200 Subject: [PATCH 2/6] Add missing tests for the setRequest var. --- src/Foundation/NSUrlSessionHandler.cs | 2 +- .../HttpClient/HttpClientTest.cs | 55 +++++++++++++++++++ tests/monotouch-test/monotouch-test.csproj | 2 + 3 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 tests/monotouch-test/HttpClient/HttpClientTest.cs diff --git a/src/Foundation/NSUrlSessionHandler.cs b/src/Foundation/NSUrlSessionHandler.cs index 8bb1aca35a9a..d8c8a4b0bc68 100644 --- a/src/Foundation/NSUrlSessionHandler.cs +++ b/src/Foundation/NSUrlSessionHandler.cs @@ -169,7 +169,7 @@ async Task CreateRequest (HttpRequestMessage request) #endif protected override async Task SendAsync (HttpRequestMessage request, CancellationToken cancellationToken) { - sentRequest = true; + Volatile.Write (ref sentRequest, true); var nsrequest = await CreateRequest (request).ConfigureAwait(false); var dataTask = session.CreateDataTask (nsrequest); diff --git a/tests/monotouch-test/HttpClient/HttpClientTest.cs b/tests/monotouch-test/HttpClient/HttpClientTest.cs new file mode 100644 index 000000000000..1f6dbdf7fa8c --- /dev/null +++ b/tests/monotouch-test/HttpClient/HttpClientTest.cs @@ -0,0 +1,55 @@ +using System; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; + +#if XAMCORE_2_0 +using Foundation; +#else +using MonoTouch.Foundation; +#endif +using NUnit.Framework; + + +namespace MonoTouchFixtures.HttpClientTests +{ + [TestFixture] + public class HttpClientTest + { + const int WaitTimeout = 5000; + + [Test] + public void CFNetworkEnsureModifiabilityPostSend () + { + var handler = new CFNetworkHandler (); + using (var client = new HttpClient (handler)) + using (var request = new HttpRequestMessage (HttpMethod.Get, "http://xamarin.com")) { + Assert.DoesNotThrow (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + Task.Factory.StartNew (() => { + var token = new CancellationTokenSource (); + client.SendAsync (request, token.Token).Wait (WaitTimeout); + Assert.Throws (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + // cancel to ensure that we do not have side effects + token.Cancel (); + }); + } + } + + [Test] + public void NSUrlSessionEnsureModifiabilityPostSend () + { + var handler = new NSUrlSessionHandler (); + using (var client = new HttpClient (handler)) + using (var request = new HttpRequestMessage (HttpMethod.Get, "http://xamarin.com")) { + Assert.DoesNotThrow (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + Task.Factory.StartNew (() => { + var token = new CancellationTokenSource (); + client.SendAsync (request, token.Token).Wait (WaitTimeout); + Assert.Throws (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + // cancel to ensure that we do not have side effects + token.Cancel (); + }); + } + } + } +} diff --git a/tests/monotouch-test/monotouch-test.csproj b/tests/monotouch-test/monotouch-test.csproj index f5b07e1c15d5..25031e81d8b3 100644 --- a/tests/monotouch-test/monotouch-test.csproj +++ b/tests/monotouch-test/monotouch-test.csproj @@ -647,6 +647,7 @@ + @@ -711,6 +712,7 @@ + From 1e09168ac2a9158195a096ab8d8596ac9b8038bb Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Date: Fri, 26 May 2017 12:58:33 +0200 Subject: [PATCH 3/6] Redesign tests to make sure that all handlers run the same code. --- .../HttpClient/HttpClientTest.cs | 88 ++++++++++++++----- 1 file changed, 68 insertions(+), 20 deletions(-) diff --git a/tests/monotouch-test/HttpClient/HttpClientTest.cs b/tests/monotouch-test/HttpClient/HttpClientTest.cs index 1f6dbdf7fa8c..df901b7090e8 100644 --- a/tests/monotouch-test/HttpClient/HttpClientTest.cs +++ b/tests/monotouch-test/HttpClient/HttpClientTest.cs @@ -1,4 +1,5 @@ -using System; +#if !__WATCHOS__ +using System; using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -18,34 +19,79 @@ public class HttpClientTest { const int WaitTimeout = 5000; - [Test] - public void CFNetworkEnsureModifiabilityPostSend () + interface IHandlerWrapper { - var handler = new CFNetworkHandler (); - using (var client = new HttpClient (handler)) - using (var request = new HttpRequestMessage (HttpMethod.Get, "http://xamarin.com")) { - Assert.DoesNotThrow (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); - Task.Factory.StartNew (() => { - var token = new CancellationTokenSource (); - client.SendAsync (request, token.Token).Wait (WaitTimeout); - Assert.Throws (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); - // cancel to ensure that we do not have side effects - token.Cancel (); - }); + bool AllowAutoRedirect { get; set; } + HttpMessageHandler Handler { get; } + + } + + // Add new classes to deal with in this class in order not to change the tests, that way we ensure all + // handlers will pass the exact same tests with no duplication. + class HandlerWrapper : IHandlerWrapper + { + string handlerType; + HttpMessageHandler handler; + + public HandlerWrapper (CFNetworkHandler handler) + { + handlerType = handler.GetType ().Name; + handler = new CFNetworkHandler (); + } + + public HandlerWrapper (NSUrlSessionHandler handler) + { + handlerType = handler.GetType ().Name; + handler = new NSUrlSessionHandler (); + } + + public bool AllowAutoRedirect + { + get + { + if (handlerType == "CFNetworkHandler") + return ((CFNetworkHandler)handler).AllowAutoRedirect; + if (handlerType == "NSUrlSessionHandler") + return ((NSUrlSessionHandler)handler).AllowAutoRedirect; + throw new InvalidOperationException (); + } + set + { + if (handlerType == "CFNetworkHandler") + ((CFNetworkHandler)handler).AllowAutoRedirect = value; + if (handlerType == "NSUrlSessionHandler") + ((NSUrlSessionHandler)handler).AllowAutoRedirect = value; + throw new InvalidOperationException (); + } + } + + public HttpMessageHandler Handler { get { return handler; } } + + public static IHandlerWrapper GetWrapper (Type handlerType) + { + switch (handlerType.Name) { + case "CFNetworkHandler": + return new HandlerWrapper (new CFNetworkHandler ()); + case "NSUrlSessionHandler": + return new HandlerWrapper (new NSUrlSessionHandler ()); + default: + throw new InvalidOperationException (); + } } } - [Test] - public void NSUrlSessionEnsureModifiabilityPostSend () + [TestCase (typeof (CFNetworkHandler))] + [TestCase (typeof (NSUrlSessionHandler))] + public void EnsureModifiabilityPostSend (Type handlerType) { - var handler = new NSUrlSessionHandler (); - using (var client = new HttpClient (handler)) + var wrapper = HandlerWrapper.GetWrapper (handlerType); + using (var client = new HttpClient (wrapper.Handler)) using (var request = new HttpRequestMessage (HttpMethod.Get, "http://xamarin.com")) { - Assert.DoesNotThrow (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + Assert.DoesNotThrow (() => wrapper.AllowAutoRedirect = !wrapper.AllowAutoRedirect); Task.Factory.StartNew (() => { var token = new CancellationTokenSource (); client.SendAsync (request, token.Token).Wait (WaitTimeout); - Assert.Throws (() => handler.AllowAutoRedirect = !handler.AllowAutoRedirect); + Assert.Throws (() => wrapper.AllowAutoRedirect = !wrapper.AllowAutoRedirect); // cancel to ensure that we do not have side effects token.Cancel (); }); @@ -53,3 +99,5 @@ public void NSUrlSessionEnsureModifiabilityPostSend () } } } +#endif + From 9bb7a48031285e751681fedc01bdda562d3e090d Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Date: Mon, 29 May 2017 12:46:30 +0200 Subject: [PATCH 4/6] Fix failing test. --- .../HttpClient/HttpClientTest.cs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/monotouch-test/HttpClient/HttpClientTest.cs b/tests/monotouch-test/HttpClient/HttpClientTest.cs index df901b7090e8..57fd1442b186 100644 --- a/tests/monotouch-test/HttpClient/HttpClientTest.cs +++ b/tests/monotouch-test/HttpClient/HttpClientTest.cs @@ -35,14 +35,14 @@ class HandlerWrapper : IHandlerWrapper public HandlerWrapper (CFNetworkHandler handler) { - handlerType = handler.GetType ().Name; - handler = new CFNetworkHandler (); + this.handlerType = handler.GetType ().Name; + this.handler = new CFNetworkHandler (); } public HandlerWrapper (NSUrlSessionHandler handler) { - handlerType = handler.GetType ().Name; - handler = new NSUrlSessionHandler (); + this.handlerType = handler.GetType ().Name; + this.handler = new NSUrlSessionHandler (); } public bool AllowAutoRedirect @@ -87,14 +87,11 @@ public void EnsureModifiabilityPostSend (Type handlerType) var wrapper = HandlerWrapper.GetWrapper (handlerType); using (var client = new HttpClient (wrapper.Handler)) using (var request = new HttpRequestMessage (HttpMethod.Get, "http://xamarin.com")) { - Assert.DoesNotThrow (() => wrapper.AllowAutoRedirect = !wrapper.AllowAutoRedirect); - Task.Factory.StartNew (() => { - var token = new CancellationTokenSource (); - client.SendAsync (request, token.Token).Wait (WaitTimeout); - Assert.Throws (() => wrapper.AllowAutoRedirect = !wrapper.AllowAutoRedirect); - // cancel to ensure that we do not have side effects - token.Cancel (); - }); + var token = new CancellationTokenSource (); + client.SendAsync (request, token.Token); + Assert.Throws (() => wrapper.AllowAutoRedirect = !wrapper.AllowAutoRedirect); + // cancel to ensure that we do not have side effects + token.Cancel (); } } } From 40e888aa9a5e209ddb54ac13e8a89217072f383b Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Date: Wed, 31 May 2017 12:56:22 +0200 Subject: [PATCH 5/6] Add the managed handler to the HttpClient tests. --- tests/monotouch-test/HttpClient/HttpClientTest.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/monotouch-test/HttpClient/HttpClientTest.cs b/tests/monotouch-test/HttpClient/HttpClientTest.cs index 57fd1442b186..0b5be8366daf 100644 --- a/tests/monotouch-test/HttpClient/HttpClientTest.cs +++ b/tests/monotouch-test/HttpClient/HttpClientTest.cs @@ -45,6 +45,12 @@ public HandlerWrapper (NSUrlSessionHandler handler) this.handler = new NSUrlSessionHandler (); } + public HandlerWrapper (HttpClientHandler handler) + { + this.handlerType = handler.GetType ().Name; + this.handler = handler; + } + public bool AllowAutoRedirect { get @@ -53,6 +59,8 @@ public bool AllowAutoRedirect return ((CFNetworkHandler)handler).AllowAutoRedirect; if (handlerType == "NSUrlSessionHandler") return ((NSUrlSessionHandler)handler).AllowAutoRedirect; + if (handlerType == "HttpClientHandler") + return ((HttpClientHandler)handler).AllowAutoRedirect; throw new InvalidOperationException (); } set @@ -61,6 +69,8 @@ public bool AllowAutoRedirect ((CFNetworkHandler)handler).AllowAutoRedirect = value; if (handlerType == "NSUrlSessionHandler") ((NSUrlSessionHandler)handler).AllowAutoRedirect = value; + if (handlerType == "HttpClientHandler") + ((HttpClientHandler)handler).AllowAutoRedirect = value; throw new InvalidOperationException (); } } @@ -74,12 +84,15 @@ public static IHandlerWrapper GetWrapper (Type handlerType) return new HandlerWrapper (new CFNetworkHandler ()); case "NSUrlSessionHandler": return new HandlerWrapper (new NSUrlSessionHandler ()); + case "HttpClientHandler": + return new HandlerWrapper (new HttpClientHandler ()); default: throw new InvalidOperationException (); } } } + [TestCase (typeof (HttpClientHandler))] [TestCase (typeof (CFNetworkHandler))] [TestCase (typeof (NSUrlSessionHandler))] public void EnsureModifiabilityPostSend (Type handlerType) From 76fa62654cf8fa923f79f13acf8b4ead0104a888 Mon Sep 17 00:00:00 2001 From: Manuel de la Pena Date: Wed, 31 May 2017 18:48:47 +0200 Subject: [PATCH 6/6] Fix minor style issues. --- .../HttpClient/HttpClientTest.cs | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/tests/monotouch-test/HttpClient/HttpClientTest.cs b/tests/monotouch-test/HttpClient/HttpClientTest.cs index 0b5be8366daf..d456d60b2357 100644 --- a/tests/monotouch-test/HttpClient/HttpClientTest.cs +++ b/tests/monotouch-test/HttpClient/HttpClientTest.cs @@ -51,10 +51,8 @@ public HandlerWrapper (HttpClientHandler handler) this.handler = handler; } - public bool AllowAutoRedirect - { - get - { + public bool AllowAutoRedirect { + get { if (handlerType == "CFNetworkHandler") return ((CFNetworkHandler)handler).AllowAutoRedirect; if (handlerType == "NSUrlSessionHandler") @@ -63,8 +61,7 @@ public bool AllowAutoRedirect return ((HttpClientHandler)handler).AllowAutoRedirect; throw new InvalidOperationException (); } - set - { + set { if (handlerType == "CFNetworkHandler") ((CFNetworkHandler)handler).AllowAutoRedirect = value; if (handlerType == "NSUrlSessionHandler") @@ -80,14 +77,14 @@ public bool AllowAutoRedirect public static IHandlerWrapper GetWrapper (Type handlerType) { switch (handlerType.Name) { - case "CFNetworkHandler": - return new HandlerWrapper (new CFNetworkHandler ()); - case "NSUrlSessionHandler": - return new HandlerWrapper (new NSUrlSessionHandler ()); - case "HttpClientHandler": - return new HandlerWrapper (new HttpClientHandler ()); - default: - throw new InvalidOperationException (); + case "CFNetworkHandler": + return new HandlerWrapper (new CFNetworkHandler ()); + case "NSUrlSessionHandler": + return new HandlerWrapper (new NSUrlSessionHandler ()); + case "HttpClientHandler": + return new HandlerWrapper (new HttpClientHandler ()); + default: + throw new InvalidOperationException (); } } }