Avoid rounding issues when checking Timeout values (#1700) (#1712)

* Avoid rounding issues when checking Timeout values (#1700)

AsTimeout is called from the SshCommand constructor with
Timeout.InfiniteTimeSpan. In this scenario the range check should never
fail, but unfortunately it does in certain scenarios, due to a runtime
or compiler bug (as soon as optimizations are turned off the issue
miraculously disappears).

Closes #1700

* fix tests

---------

Co-authored-by: Robert Hague <rh@johnstreetcapital.com>

src/Renci.SshNet/Common/TimeSpanExtensions.cs

@@ -19,9 +19,9 @@ namespace Renci.SshNet.Common
         /// </exception>
         public static int AsTimeout(this TimeSpan timeSpan, [CallerArgumentExpression(nameof(timeSpan))] string? paramName = null)
         {
-            var timeoutInMilliseconds = timeSpan.TotalMilliseconds;
-            return timeoutInMilliseconds is < -1d or > int.MaxValue
-                       ? throw new ArgumentOutOfRangeException(paramName, "The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.")
+            var timeoutInMilliseconds = (long)timeSpan.TotalMilliseconds;
+            return timeoutInMilliseconds is < -1 or > int.MaxValue
+                       ? throw new ArgumentOutOfRangeException(paramName, timeSpan, "The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.")
                        : (int)timeoutInMilliseconds;
         }
 

test/Renci.SshNet.Tests/Classes/Common/TimeSpanExtensionsTest.cs

@@ -3,7 +3,6 @@
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 using Renci.SshNet.Common;
-using Renci.SshNet.Tests.Common;
 
 namespace Renci.SshNet.Tests.Classes.Common
 {
@@ -21,34 +20,17 @@ namespace Renci.SshNet.Tests.Classes.Common
         }
 
         [TestMethod]
-        public void AsTimeout_NegativeTimeSpan_ThrowsArgumentOutOfRangeException()
+        [DataRow(-2)]
+        [DataRow((double)int.MaxValue + 1)]
+        public void AsTimeout_InvalidTimeSpan_ThrowsArgumentOutOfRangeException(double milliseconds)
         {
-            var timeSpan = TimeSpan.FromSeconds(-1);
-            Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.AsTimeout());
-        }
+            var timeSpan = TimeSpan.FromMilliseconds(milliseconds);
 
-        [TestMethod]
-        public void AsTimeout_TimeSpanExceedingMaxValue_ThrowsArgumentOutOfRangeException()
-        {
-            var timeSpan = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.AsTimeout());
-        }
+            var ex = Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.AsTimeout());
 
-        [TestMethod]
-        public void AsTimeout_ArgumentOutOfRangeException_HasCorrectInformation()
-        {
-            var timeSpan = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            try
-            {
+            Assert.Contains("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex.Message, StringComparison.Ordinal);
 
-                timeSpan.AsTimeout();
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-                Assert.AreEqual(nameof(timeSpan), ex.ParamName);
-            }
+            Assert.AreEqual(nameof(timeSpan), ex.ParamName);
         }
 
         [TestMethod]
@@ -60,34 +42,17 @@ namespace Renci.SshNet.Tests.Classes.Common
         }
 
         [TestMethod]
-        public void EnsureValidTimeout_NegativeTimeSpan_ThrowsArgumentOutOfRangeException()
+        [DataRow(-2)]
+        [DataRow((double)int.MaxValue + 1)]
+        public void EnsureValidTimeout_InvalidTimeSpan_ThrowsArgumentOutOfRangeException(double milliseconds)
         {
-            var timeSpan = TimeSpan.FromSeconds(-1);
-            Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.EnsureValidTimeout());
-        }
+            var timeSpan = TimeSpan.FromMilliseconds(milliseconds);
 
-        [TestMethod]
-        public void EnsureValidTimeout_TimeSpanExceedingMaxValue_ThrowsArgumentOutOfRangeException()
-        {
-            var timeSpan = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.EnsureValidTimeout());
-        }
+            var ex = Assert.ThrowsExactly<ArgumentOutOfRangeException>(() => timeSpan.EnsureValidTimeout());
 
-        [TestMethod]
-        public void EnsureValidTimeout_ArgumentOutOfRangeException_HasCorrectInformation()
-        {
-            var timeSpan = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            try
-            {
+            Assert.Contains("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex.Message, StringComparison.Ordinal);
 
-                timeSpan.EnsureValidTimeout();
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-                Assert.AreEqual(nameof(timeSpan), ex.ParamName);
-            }
+            Assert.AreEqual(nameof(timeSpan), ex.ParamName);
         }
     }
 }

test/Renci.SshNet.Tests/Classes/ConnectionInfoTest.cs

@@ -286,35 +286,17 @@ namespace Renci.SshNet.Tests.Classes
                                                     Resources.HOST, int.Parse(Resources.PORT), Resources.USERNAME,
                                                     Resources.PASSWORD, new KeyboardInteractiveAuthenticationMethod(Resources.USERNAME));
 
-            try
-            {
-                connectionInfo.Timeout = TimeSpan.FromMilliseconds(-2);
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => connectionInfo.Timeout = TimeSpan.FromMilliseconds(-2));
+            Assert.AreEqual("Timeout", ex.ParamName);
 
-                Assert.AreEqual("Timeout", ex.ParamName);
-            }
+            ex = Assert.Throws<ArgumentOutOfRangeException>(() => connectionInfo.Timeout = TimeSpan.FromMilliseconds((double)int.MaxValue + 1));
+            Assert.AreEqual("Timeout", ex.ParamName);
 
             connectionInfo.Timeout = TimeSpan.FromMilliseconds(-1);
             Assert.AreEqual(connectionInfo.Timeout, TimeSpan.FromMilliseconds(-1));
 
             connectionInfo.Timeout = TimeSpan.FromMilliseconds(int.MaxValue);
             Assert.AreEqual(connectionInfo.Timeout, TimeSpan.FromMilliseconds(int.MaxValue));
-
-            try
-            {
-                connectionInfo.Timeout = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("Timeout", ex.ParamName);
-            }
         }
 
         [TestMethod]
@@ -325,35 +307,17 @@ namespace Renci.SshNet.Tests.Classes
                                                     Resources.HOST, int.Parse(Resources.PORT), Resources.USERNAME,
                                                     Resources.PASSWORD, new KeyboardInteractiveAuthenticationMethod(Resources.USERNAME));
 
-            try
-            {
-                connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds(-2);
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds(-2));
+            Assert.AreEqual("ChannelCloseTimeout", ex.ParamName);
 
-                Assert.AreEqual("ChannelCloseTimeout", ex.ParamName);
-            }
+            ex = Assert.Throws<ArgumentOutOfRangeException>(() => connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds((double)int.MaxValue + 1));
+            Assert.AreEqual("ChannelCloseTimeout", ex.ParamName);
 
             connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds(-1);
             Assert.AreEqual(connectionInfo.ChannelCloseTimeout, TimeSpan.FromMilliseconds(-1));
 
             connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds(int.MaxValue);
             Assert.AreEqual(connectionInfo.ChannelCloseTimeout, TimeSpan.FromMilliseconds(int.MaxValue));
-
-            try
-            {
-                connectionInfo.ChannelCloseTimeout = TimeSpan.FromMilliseconds((double)int.MaxValue + 1);
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("ChannelCloseTimeout", ex.ParamName);
-            }
         }
 
         [TestMethod]

test/Renci.SshNet.Tests/Classes/NetConfClientTest.cs

@@ -80,17 +80,8 @@ namespace Renci.SshNet.Tests.Classes
             var connectionInfo = new PasswordConnectionInfo("host", 22, "admin", "pwd");
             var target = new NetConfClient(connectionInfo);
 
-            try
-            {
-                target.OperationTimeout = operationTimeout;
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("OperationTimeout", ex.ParamName);
-            }
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => target.OperationTimeout = operationTimeout);
+            Assert.AreEqual("OperationTimeout", ex.ParamName);
         }
 
         [TestMethod]
@@ -100,17 +91,8 @@ namespace Renci.SshNet.Tests.Classes
             var connectionInfo = new PasswordConnectionInfo("host", 22, "admin", "pwd");
             var target = new NetConfClient(connectionInfo);
 
-            try
-            {
-                target.OperationTimeout = operationTimeout;
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("OperationTimeout", ex.ParamName);
-            }
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => target.OperationTimeout = operationTimeout);
+            Assert.AreEqual("OperationTimeout", ex.ParamName);
         }
     }
 }

test/Renci.SshNet.Tests/Classes/SftpClientTest.cs

@@ -83,17 +83,8 @@ namespace Renci.SshNet.Tests.Classes
             var connectionInfo = new PasswordConnectionInfo("host", 22, "admin", "pwd");
             var target = new SftpClient(connectionInfo);
 
-            try
-            {
-                target.OperationTimeout = operationTimeout;
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("OperationTimeout", ex.ParamName);
-            }
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => target.OperationTimeout = operationTimeout);
+            Assert.AreEqual("OperationTimeout", ex.ParamName);
         }
 
         [TestMethod]
@@ -103,17 +94,8 @@ namespace Renci.SshNet.Tests.Classes
             var connectionInfo = new PasswordConnectionInfo("host", 22, "admin", "pwd");
             var target = new SftpClient(connectionInfo);
 
-            try
-            {
-                target.OperationTimeout = operationTimeout;
-            }
-            catch (ArgumentOutOfRangeException ex)
-            {
-                Assert.IsNull(ex.InnerException);
-                ArgumentExceptionAssert.MessageEquals("The timeout must represent a value between -1 and Int32.MaxValue milliseconds, inclusive.", ex);
-
-                Assert.AreEqual("OperationTimeout", ex.ParamName);
-            }
+            var ex = Assert.Throws<ArgumentOutOfRangeException>(() => target.OperationTimeout = operationTimeout);
+            Assert.AreEqual("OperationTimeout", ex.ParamName);
         }
     }
 }