From 1d67144c8b0e61fb8f90c1b635ec4e811e81b445 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Dec 2020 14:39:52 +0100 Subject: [PATCH 1/5] extend ReceiveDataTimeout functional coverage --- .../tests/UnitTests/APICallHistory.cs | 2 +- .../tests/UnitTests/FakeInterop.cs | 4 ++++ .../tests/UnitTests/WinHttpHandlerTest.cs | 16 ++++++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs index 961cb2ffbad545..b50296edc23e83 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/APICallHistory.cs @@ -66,7 +66,7 @@ public static ProxyInfo RequestProxySettings public static int? WinHttpOptionSendTimeout { get; set; } - public static int? WinHttpOptionReceiveTimeout { get; set; } + public static uint? WinHttpOptionReceiveTimeout { get; set; } public static List WinHttpOptionClientCertContext { get { return winHttpOptionClientCertContextList; } } diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/FakeInterop.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/FakeInterop.cs index a92c0124d9e5cb..596ce8212979b0 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/FakeInterop.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/FakeInterop.cs @@ -507,6 +507,10 @@ public static bool WinHttpSetOption( { APICallHistory.WinHttpOptionRedirectPolicy = optionData; } + else if (option == Interop.WinHttp.WINHTTP_OPTION_RECEIVE_TIMEOUT) + { + APICallHistory.WinHttpOptionReceiveTimeout = optionData; + } return true; } diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs index 0d7e4622d98d51..2cdd62b18dbd59 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs @@ -463,6 +463,16 @@ public void MaxConnectionsPerServer_SetPositiveValue_Success() handler.MaxConnectionsPerServer = 1; } + [Fact] + public void ReceiveDataTimeout_SetValidValue_ForwardsCorrectNativeOptionToWinHttp() + { + var handler = new WinHttpHandler(); + + SendRequestHelper.Send(handler, () => handler.ReceiveDataTimeout = TimeSpan.FromSeconds(13)); + + Assert.Equal(13_000u, APICallHistory.WinHttpOptionReceiveTimeout.Value); + } + [Fact] public void ReceiveDataTimeout_SetNegativeValue_ThrowsArgumentOutOfRangeException() { @@ -490,11 +500,13 @@ public void ReceiveDataTimeout_SetZeroValue_ThrowsArgumentOutOfRangeException() } [Fact] - public void ReceiveDataTimeout_SetInfiniteValue_NoExceptionThrown() + public void ReceiveDataTimeout_SetInfiniteTimeSpan_TranslatesToUInt32MaxValue() { var handler = new WinHttpHandler(); - handler.ReceiveDataTimeout = Timeout.InfiniteTimeSpan; + SendRequestHelper.Send(handler, () => handler.ReceiveDataTimeout = Timeout.InfiniteTimeSpan); + + Assert.Equal(uint.MaxValue, APICallHistory.WinHttpOptionReceiveTimeout.Value); } [Fact] From 94bc901bbb1c766c5918bdb8ae820c896e3ce668 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Dec 2020 14:55:23 +0100 Subject: [PATCH 2/5] fix double -> uint conversions --- .../src/System/Net/Http/WinHttpHandler.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 1525bc936e01dc..8333feb08415b0 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -962,7 +962,10 @@ private async Task StartRequestAsync(WinHttpRequestState state) // Since the headers have been read, set the "receive" timeout to be based on each read // call of the response body data. WINHTTP_OPTION_RECEIVE_TIMEOUT sets a timeout on each // lower layer winsock read. - uint optionData = unchecked((uint)_receiveDataTimeout.TotalMilliseconds); + // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days). + // The result a of double->uint cast is unspecified and may differ on ARM. + // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. + uint optionData = unchecked((uint)(int)_receiveDataTimeout.TotalMilliseconds); SetWinHttpOption(state.RequestHandle, Interop.WinHttp.WINHTTP_OPTION_RECEIVE_TIMEOUT, ref optionData); HttpResponseMessage responseMessage = @@ -1007,8 +1010,10 @@ private unsafe void SetTcpKeepalive(SafeWinHttpHandle sessionHandle) onoff = 1, // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days) - keepaliveinterval = (uint)_tcpKeepAliveInterval.TotalMilliseconds, - keepalivetime = (uint)_tcpKeepAliveTime.TotalMilliseconds + // The result a of double->uint cast is unspecified and may differ on ARM. + // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. + keepaliveinterval = unchecked((uint)(int)_tcpKeepAliveInterval.TotalMilliseconds), + keepalivetime = unchecked((uint)(int)_tcpKeepAliveTime.TotalMilliseconds) }; SetWinHttpOption( From 67f53a0dce7c08f80acb1e4d0f2810ad5fc208da Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Dec 2020 16:51:09 +0100 Subject: [PATCH 3/5] more comments + remove 'unchecked' --- .../src/System/Net/Http/WinHttpHandler.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 8333feb08415b0..847b488be5969f 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -963,9 +963,9 @@ private async Task StartRequestAsync(WinHttpRequestState state) // call of the response body data. WINHTTP_OPTION_RECEIVE_TIMEOUT sets a timeout on each // lower layer winsock read. // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days). - // The result a of double->uint cast is unspecified and may differ on ARM. + // The result a of double->uint cast is unspecified and may differ on ARM, returning 0 instead of uint.MaxValue. // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. - uint optionData = unchecked((uint)(int)_receiveDataTimeout.TotalMilliseconds); + uint optionData = (uint)(int)_receiveDataTimeout.TotalMilliseconds; SetWinHttpOption(state.RequestHandle, Interop.WinHttp.WINHTTP_OPTION_RECEIVE_TIMEOUT, ref optionData); HttpResponseMessage responseMessage = @@ -1010,10 +1010,10 @@ private unsafe void SetTcpKeepalive(SafeWinHttpHandle sessionHandle) onoff = 1, // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days) - // The result a of double->uint cast is unspecified and may differ on ARM. + // The result a of double->uint cast is unspecified and may differ on ARM, returning 0 instead of uint.MaxValue. // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. - keepaliveinterval = unchecked((uint)(int)_tcpKeepAliveInterval.TotalMilliseconds), - keepalivetime = unchecked((uint)(int)_tcpKeepAliveTime.TotalMilliseconds) + keepaliveinterval = (uint)(int)_tcpKeepAliveInterval.TotalMilliseconds, + keepalivetime = (uint)(int)_tcpKeepAliveTime.TotalMilliseconds }; SetWinHttpOption( From 8aab7d7aded5cc3ca112c7ad3e53ab1eeaaa252a Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Dec 2020 19:31:43 +0100 Subject: [PATCH 4/5] Comment suggestion Co-authored-by: Jan Kotas --- .../src/System/Net/Http/WinHttpHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 847b488be5969f..6d3e9330bab2ac 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -963,8 +963,8 @@ private async Task StartRequestAsync(WinHttpRequestState state) // call of the response body data. WINHTTP_OPTION_RECEIVE_TIMEOUT sets a timeout on each // lower layer winsock read. // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days). - // The result a of double->uint cast is unspecified and may differ on ARM, returning 0 instead of uint.MaxValue. - // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. + // The result a of double->uint cast is unspecified for -1 and may differ on ARM, returning 0 instead of uint.MaxValue. + // To handle Timeout.InfiniteTimespan correctly, we need to cast to int first. uint optionData = (uint)(int)_receiveDataTimeout.TotalMilliseconds; SetWinHttpOption(state.RequestHandle, Interop.WinHttp.WINHTTP_OPTION_RECEIVE_TIMEOUT, ref optionData); From e747945ea5c29d65ca07fc8fac0daa87442a68ac Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Dec 2020 19:31:54 +0100 Subject: [PATCH 5/5] Comment suggestion Co-authored-by: Jan Kotas --- .../src/System/Net/Http/WinHttpHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 6d3e9330bab2ac..85a12519030afe 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -1010,8 +1010,8 @@ private unsafe void SetTcpKeepalive(SafeWinHttpHandle sessionHandle) onoff = 1, // Timeout.InfiniteTimeSpan will be converted to uint.MaxValue milliseconds (~ 50 days) - // The result a of double->uint cast is unspecified and may differ on ARM, returning 0 instead of uint.MaxValue. - // To handle Timeout.InfiniteTimespan correctly, we need to cast to Int32 first. + // The result a of double->uint cast is unspecified for -1 and may differ on ARM, returning 0 instead of uint.MaxValue. + // To handle Timeout.InfiniteTimespan correctly, we need to cast to int first. keepaliveinterval = (uint)(int)_tcpKeepAliveInterval.TotalMilliseconds, keepalivetime = (uint)(int)_tcpKeepAliveTime.TotalMilliseconds };