Add support for configuring OperationTimeout in the connection string (#644)

* Add support for configuring OperationTimeout in the connection string

Like in [Microsoft.ServiceBus.Messaging (.NET Framework)][1], it's now possible to configure the `OperationTimeout` in the connection string.

The `ServiceBusConnection` class constructors are adapted to read the `OperationTimeout` from the connection string and the constructor explicitly having a `TimeSpan operationTimeout` argument is obsoleted with this message:
> Please use the constructor with (string namespaceConnectionString, RetryPolicy retryPolicy) arguments and define the operationTimeout in the connection string.

[1]: https://docs.microsoft.com/en-us/dotnet/api/microsoft.servicebus.servicebusconnectionstringbuilder.operationtimeout

* Use the operation timeout defined in the connection string for the ManagementClient

Also, **actually** use the operation timeout as a timeout for the `HttpClient` responsible for the management operations instead of storing the `Constants.DefaultOperationTimeout` in a private field which is never used.

* Update API Approvals

* Always append key value pair delimiter when building the connection string

* Improve wording as suggested by @SeanFeldman

* Improve operation timeout parsing

Parse as int (seconds) first, fallback on TimeSpan parsing for compatibility with WindowsAzure.ServiceBus.

* Throw if the operation timeout input is neither parsable as int or TimeSpan.
* Throw if the operation timeout is smaller than or equal to zero.
* Throw if the operation timeout is greater than or equal to one hour.

Author: 0xced

src/Microsoft.Azure.ServiceBus/Management/ManagementClient.cs

@@ -18,7 +18,6 @@ public class ManagementClient
         private HttpClient httpClient;
         private readonly string endpointFQDN;
         private readonly ITokenProvider tokenProvider;
-        private readonly TimeSpan operationTimeout;
         private readonly int port;
         private readonly string clientId;
 
@@ -48,14 +47,13 @@ public ManagementClient(string endpoint, ITokenProvider tokenProvider)
         /// <param name="tokenProvider">Token provider which will generate security tokens for authorization.</param>
         public ManagementClient(ServiceBusConnectionStringBuilder connectionStringBuilder, ITokenProvider tokenProvider = default)
         {
-            this.httpClient = new HttpClient();
+            this.httpClient = new HttpClient { Timeout = connectionStringBuilder.OperationTimeout };
             this.endpointFQDN = connectionStringBuilder.Endpoint;
             this.tokenProvider = tokenProvider ?? CreateTokenProvider(connectionStringBuilder);
-            this.operationTimeout = Constants.DefaultOperationTimeout;
             this.port = GetPort(connectionStringBuilder.Endpoint);
             this.clientId = nameof(ManagementClient) + Guid.NewGuid().ToString("N").Substring(0, 6);
 
-            MessagingEventSource.Log.ManagementClientCreated(this.clientId, this.operationTimeout.TotalSeconds, this.tokenProvider.ToString());
+            MessagingEventSource.Log.ManagementClientCreated(this.clientId, this.httpClient.Timeout.TotalSeconds, this.tokenProvider.ToString());
         }
 
         public static HttpRequestMessage CloneRequest(HttpRequestMessage req)

src/Microsoft.Azure.ServiceBus/ServiceBusConnection.cs

@@ -37,19 +37,43 @@ public ServiceBusConnection(ServiceBusConnectionStringBuilder connectionStringBu
         /// <param name="namespaceConnectionString">Namespace connection string</param>
         /// <remarks>It is the responsibility of the user to close the connection after use through <see cref="CloseAsync"/></remarks>
         public ServiceBusConnection(string namespaceConnectionString)
-            : this(namespaceConnectionString, Constants.DefaultOperationTimeout, RetryPolicy.Default)
+            : this(namespaceConnectionString, RetryPolicy.Default)
         {
         }
 
+        /// <summary>
+        /// Creates a new connection to service bus.
+        /// </summary>
+        /// <param name="namespaceConnectionString">Namespace connection string.</param>
+        /// <param name="retryPolicy">Retry policy for operations. Defaults to <see cref="RetryPolicy.Default"/></param>
+        /// <remarks>It is the responsibility of the user to close the connection after use through <see cref="CloseAsync"/></remarks>
+        public ServiceBusConnection(string namespaceConnectionString, RetryPolicy retryPolicy = null)
+            : this(retryPolicy)
+        {
+            if (string.IsNullOrWhiteSpace(namespaceConnectionString))
+            {
+                throw Fx.Exception.ArgumentNullOrWhiteSpace(nameof(namespaceConnectionString));
+            }
+
+            var serviceBusConnectionStringBuilder = new ServiceBusConnectionStringBuilder(namespaceConnectionString);
+            if (!string.IsNullOrWhiteSpace(serviceBusConnectionStringBuilder.EntityPath))
+            {
+                throw Fx.Exception.Argument(nameof(namespaceConnectionString), "NamespaceConnectionString should not contain EntityPath.");
+            }
+
+            this.InitializeConnection(serviceBusConnectionStringBuilder);
+        }
+
         /// <summary>
         /// Creates a new connection to service bus.
         /// </summary>
         /// <param name="namespaceConnectionString">Namespace connection string.</param>
         /// <param name="operationTimeout">Duration after which individual operations will timeout.</param>
         /// <param name="retryPolicy">Retry policy for operations. Defaults to <see cref="RetryPolicy.Default"/></param>
         /// <remarks>It is the responsibility of the user to close the connection after use through <see cref="CloseAsync"/></remarks>
+        [Obsolete("This constructor is obsolete. Use ServiceBusConnection(string namespaceConnectionString, RetryPolicy retryPolicy) constructor instead, providing operationTimeout in the connection string.")]
         public ServiceBusConnection(string namespaceConnectionString, TimeSpan operationTimeout, RetryPolicy retryPolicy = null)
-            : this(operationTimeout, retryPolicy)
+            : this(retryPolicy)
         {
             if (string.IsNullOrWhiteSpace(namespaceConnectionString))
             {
@@ -63,6 +87,8 @@ public ServiceBusConnection(string namespaceConnectionString, TimeSpan operation
             }
 
             this.InitializeConnection(serviceBusConnectionStringBuilder);
+            // operationTimeout argument explicitly provided by caller should take precedence over OperationTimeout found in the connection string.
+            this.OperationTimeout = operationTimeout;
         }
 
         /// <summary>
@@ -72,7 +98,7 @@ public ServiceBusConnection(string namespaceConnectionString, TimeSpan operation
         /// <param name="transportType">Transport type.</param>
         /// <param name="retryPolicy">Retry policy for operations. Defaults to <see cref="RetryPolicy.Default"/></param>
         public ServiceBusConnection(string endpoint, TransportType transportType, RetryPolicy retryPolicy = null)
-            : this(Constants.DefaultOperationTimeout, retryPolicy)
+            : this(retryPolicy)
         {
             if (string.IsNullOrWhiteSpace(endpoint))
             {
@@ -88,9 +114,8 @@ public ServiceBusConnection(string endpoint, TransportType transportType, RetryP
             this.InitializeConnection(serviceBusConnectionStringBuilder);
         }
 
-        internal ServiceBusConnection(TimeSpan operationTimeout, RetryPolicy retryPolicy = null)
+        internal ServiceBusConnection(RetryPolicy retryPolicy = null)
         {
-            this.OperationTimeout = operationTimeout;
             this.RetryPolicy = retryPolicy ?? RetryPolicy.Default;
             this.syncLock = new object();
         }
@@ -193,6 +218,7 @@ void InitializeConnection(ServiceBusConnectionStringBuilder builder)
                 this.TokenProvider = new SharedAccessSignatureTokenProvider(builder.SasKeyName, builder.SasKey);
             }
 
+            this.OperationTimeout = builder.OperationTimeout;
             this.TransportType = builder.TransportType;
             this.ConnectionManager = new FaultTolerantAmqpObject<AmqpConnection>(this.CreateConnectionAsync, CloseConnection);
             this.TransactionController = new FaultTolerantAmqpObject<Controller>(this.CreateControllerAsync, CloseController);

src/Microsoft.Azure.ServiceBus/ServiceBusConnectionStringBuilder.cs

@@ -5,6 +5,7 @@ namespace Microsoft.Azure.ServiceBus
 {
     using System;
     using System.Collections.Generic;
+    using System.Globalization;
     using System.Text;
     using Primitives;
 
@@ -24,6 +25,8 @@ public class ServiceBusConnectionStringBuilder
         const string EntityPathConfigName = "EntityPath";
         const string TransportTypeConfigName = "TransportType";
 
+        const string OperationTimeoutConfigName = "OperationTimeout";
+
         string entityPath, sasKeyName, sasKey, sasToken, endpoint;
 
         /// <summary>
@@ -212,6 +215,12 @@ public string SasToken
         /// </summary>
         public TransportType TransportType { get; set; }
 
+        /// <summary>
+        /// Duration after which individual operations will timeout.
+        /// </summary>
+        /// <remarks>Defaults to 1 minute.</remarks>
+        public TimeSpan OperationTimeout { get; set; } = Constants.DefaultOperationTimeout;
+
         internal Dictionary<string, string> ConnectionStringProperties = new Dictionary<string, string>(StringComparer.CurrentCultureIgnoreCase);
 
         /// <summary>
@@ -243,7 +252,12 @@ public string GetNamespaceConnectionString()
 
             if (this.TransportType != TransportType.Amqp)
             {
-                connectionStringBuilder.Append($"{TransportTypeConfigName}{KeyValueSeparator}{this.TransportType}");
+                connectionStringBuilder.Append($"{TransportTypeConfigName}{KeyValueSeparator}{this.TransportType}{KeyValuePairDelimiter}");
+            }
+
+            if (this.OperationTimeout != Constants.DefaultOperationTimeout)
+            {
+                connectionStringBuilder.Append($"{OperationTimeoutConfigName}{KeyValueSeparator}{this.OperationTimeout}{KeyValuePairDelimiter}");
             }
 
             return connectionStringBuilder.ToString().Trim(';');
@@ -319,6 +333,31 @@ void ParseConnectionString(string connectionString)
                         this.TransportType = transportType;
                     }
                 }
+                else if (key.Equals(OperationTimeoutConfigName, StringComparison.OrdinalIgnoreCase))
+                {
+                    if (int.TryParse(value, NumberStyles.Integer, NumberFormatInfo.InvariantInfo, out var timeoutInSeconds))
+                    {
+                        this.OperationTimeout = TimeSpan.FromSeconds(timeoutInSeconds);
+                    }
+                    else if (TimeSpan.TryParse(value, NumberFormatInfo.InvariantInfo, out var operationTimeout))
+                    {
+                        this.OperationTimeout = operationTimeout;
+                    }
+                    else
+                    {
+                        throw Fx.Exception.Argument(nameof(connectionString), $"The {OperationTimeoutConfigName} ({value}) format is invalid. It must be an integer representing a number of seconds.");
+                    }
+
+                    if (this.OperationTimeout.TotalMilliseconds <= 0)
+                    {
+                        throw Fx.Exception.Argument(nameof(connectionString), $"The {OperationTimeoutConfigName} ({value}) must be greater than zero.");
+                    }
+
+                    if (this.OperationTimeout.TotalHours >= 1)
+                    {
+                        throw Fx.Exception.Argument(nameof(connectionString), $"The {OperationTimeoutConfigName} ({value}) must be smaller than one hour.");
+                    }
+                }
                 else
                 {
                     ConnectionStringProperties[key] = value;

test/Microsoft.Azure.ServiceBus.UnitTests/API/ApiApprovals.ApproveAzureServiceBus.approved.txt

@@ -309,6 +309,10 @@ namespace Microsoft.Azure.ServiceBus
     {
         public ServiceBusConnection(Microsoft.Azure.ServiceBus.ServiceBusConnectionStringBuilder connectionStringBuilder) { }
         public ServiceBusConnection(string namespaceConnectionString) { }
+        public ServiceBusConnection(string namespaceConnectionString, Microsoft.Azure.ServiceBus.RetryPolicy retryPolicy = null) { }
+        [System.ObsoleteAttribute("This constructor is obsolete. Use ServiceBusConnection(string namespaceConnection" +
+            "String, RetryPolicy retryPolicy) constructor instead, providing operationTimeout" +
+            " in the connection string.")]
         public ServiceBusConnection(string namespaceConnectionString, System.TimeSpan operationTimeout, Microsoft.Azure.ServiceBus.RetryPolicy retryPolicy = null) { }
         public ServiceBusConnection(string endpoint, Microsoft.Azure.ServiceBus.TransportType transportType, Microsoft.Azure.ServiceBus.RetryPolicy retryPolicy = null) { }
         public System.Uri Endpoint { get; set; }
@@ -329,6 +333,7 @@ namespace Microsoft.Azure.ServiceBus
         public ServiceBusConnectionStringBuilder(string endpoint, string entityPath, string sharedAccessSignature, Microsoft.Azure.ServiceBus.TransportType transportType) { }
         public string Endpoint { get; set; }
         public string EntityPath { get; set; }
+        public System.TimeSpan OperationTimeout { get; set; }
         public string SasKey { get; set; }
         public string SasKeyName { get; set; }
         public string SasToken { get; set; }

test/Microsoft.Azure.ServiceBus.UnitTests/ServiceBusConnectionStringBuilderTests.cs

@@ -75,6 +75,13 @@ void ConnectionStringBuilderShouldTrimTrailingSemicolon()
 
             csBuilder.EntityPath = "myQ";
             Assert.Equal("Endpoint=amqps://contoso.servicebus.windows.net;EntityPath=myQ", csBuilder.ToString());
+
+            csBuilder.EntityPath = "";
+            csBuilder.TransportType = TransportType.AmqpWebSockets;
+            Assert.Equal("Endpoint=amqps://contoso.servicebus.windows.net;TransportType=AmqpWebSockets", csBuilder.ToString());
+
+            csBuilder.OperationTimeout = TimeSpan.FromSeconds(42);
+            Assert.Equal("Endpoint=amqps://contoso.servicebus.windows.net;TransportType=AmqpWebSockets;OperationTimeout=00:00:42", csBuilder.ToString());
         }
 
         [Fact]
@@ -126,6 +133,36 @@ void ConnectionStringBuilderShouldDefaultToAmqp()
             Assert.Equal(TransportType.Amqp, csBuilder.TransportType);
         }
 
+        [Fact]
+        void ConnectionStringBuilderShouldParseOperationTimeoutAsInteger()
+        {
+            var csBuilder = new ServiceBusConnectionStringBuilder("Endpoint=sb://contoso.servicebus.windows.net;SharedAccessKeyName=keyname;SharedAccessKey=key;OperationTimeout=120");
+            Assert.Equal(TimeSpan.FromMinutes(2), csBuilder.OperationTimeout);
+        }
+
+        [Fact]
+        void ConnectionStringBuilderShouldParseOperationTimeoutAsTimeSpan()
+        {
+            var csBuilder = new ServiceBusConnectionStringBuilder("Endpoint=sb://contoso.servicebus.windows.net;SharedAccessKeyName=keyname;SharedAccessKey=key;OperationTimeout=00:12:34");
+            Assert.Equal(TimeSpan.FromMinutes(12).Add(TimeSpan.FromSeconds(34)), csBuilder.OperationTimeout);
+        }
+
+        [Fact]
+        void ConnectionStringBuilderOperationTimeoutShouldDefaultToOneMinute()
+        {
+            var csBuilder = new ServiceBusConnectionStringBuilder("Endpoint=sb://contoso.servicebus.windows.net;SharedAccessKeyName=keyname;SharedAccessKey=key");
+            Assert.Equal(Constants.DefaultOperationTimeout, csBuilder.OperationTimeout);
+        }
+
+        [Fact]
+        void ConnectionStringBuilderShouldThrowForInvalidOperationTimeout()
+        {
+            var exception = Assert.Throws<ArgumentException>(() => new ServiceBusConnectionStringBuilder("Endpoint=sb://contoso.servicebus.windows.net;SharedAccessKeyName=keyname;SharedAccessKey=key;OperationTimeout=x"));
+            Assert.Equal("connectionString", exception.ParamName);
+            Assert.Contains("OperationTimeout", exception.Message);
+            Assert.Contains("(x)", exception.Message);
+        }
+
         [Fact]
         void ConnectionStringBuilderShouldParseToken()
         {