Add TimeSpan timeouts

[Wraith2]

In https://github.com/dotnet/runtime some new apis were approved, dotnet/runtime#14336 , to add TimeSpan timeouts in the BCL where it was appropriate and some of those locations were in SqlClient, SqlClient in the BCL is now in maintenance mode but we can implement those new apis in this library.

Having to use second or millisecond int timeouts has always been a source of bugs and having timeouts be more naturally expressible will feel cleaner for very little cost.

The approved apis that apply to this library were:

namespace System.Data.Sql {
    public sealed class SqlNotificationRequest {
+       public TimeSpan TimeoutTimeSpan { get; set; }
    }
}

namespace System.Data.SqlClient {
    public sealed class SqlBulkCopy {
+       public TimeSpan BulkCopyTimeoutTimeSpan { get; set; }
    }

    public sealed class SqlConnectionStringBuilder {
+       public TimeSpan ConnectTimeoutTimeSpan { get; set; }
+       public TimeSpan LoadBalanceTimeoutTimeSpan { get; set; }
+       public TimeSpan CommandTimeoutTimeSpan { get; set; }
    }

    public sealed class SqlDependency {
+       public SqlDependency(SqlCommand command, string options, TimeSpan timeout);
    }
}

We should also consider adding something to SqlCommand to cover this part:

namespace System.Data {
!   This would need to be a DIM.
    public interface IDbCommand {
+       TimeSpan CommandTimeoutTimeSpan { get; set; }
    }
}

[cheenamalhotra]

Before we consider updating independently maintained APIs, since IDbCommand is not ours I guess SqlCommand would have to wait for dotnet/runtime to release that support. Ideally a change like that should go altogether so there are no more "int" timespans in the driver, otherwise partial breaking changes on the same behavior are not good for user experience.

So technically, we're going to depend on dotnet/runtime#14336 to be implemented and shipped in a stable release before we can release it. And as there are no serious concerns or demands, it does not seem high priority.

[Wraith2]

For SqlCommand we can add our own CommandTimeoutTimeSpan implementation and then make it an override for whatever version of the runtime eventually includes it. For the majority of things we can own the types so we can add the members on whatever timeframe we choose.

It isn't high priority but external users like me have no visibility of the priority on issues so there's no indication of what's planned for 4.0.0 or beyond.

[roji]

Note the API review decision in dotnet/runtime#14336 (comment) which approves adding method overloads, but rejects adding properties and interface DIMs (incidentally I agree). Of course, you can decide to do something else in SqlClient, but those API designers generally know what they're doing - unless you think the ADO.NET timeout properties are somehow different?

So in general I'm not a big fan... Also, though I completely agree that the current int-based are an API smell, in practice I've seen little actual confusion over the years - users seem to look at the Intellisense docs and get it right.

Specifically for SqlConnectionStringBuilder, don't forget that you also have the two timeout parameters in the connection string, which already work with seconds. I'm not sure it's a great idea to add a connection string parameter with a TimeSpan string representation (e.g. 00:00:30 for 30 seconds?); and unless you do that, you probably shouldn't add TimeSpan properties to SqlConnectionStringBuilder (since properties on that type are supposed to correspond to string-based parameters).

[ErikEJ]

SqlConnectionStringBuilder.CommandTimeout ?

[Wraith2]

SqlConnectionStringBuilder.CommandTimeout ?

Added.