Repository navigation
[API Proposal]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 2+3) #67156
Closed as not planned
Description
Background and motivation
As discussed with @danmoseley this issue serves as a follow up for #14336 and is meant to represent the 2nd and 3rd group
API Proposal
Group 2
namespace System.IO {
public abstract class Stream {
public TimeSpan ReadTimeoutTimeSpan { get; set; }
public TimeSpan WriteTimeoutTimeSpan { get; set; }
}
}
namespace System.IO.Ports {
public class SerialPort {
public TimeSpan ReadTimeoutTimeSpan { get; set; }
public TimeSpan WriteTimeoutTimeSpan { get; set; }
}
}
namespace System.Media {
public class SoundPlayer {
public TimeSpan LoadTimeoutTimeSpan { get; set; }
}
}
namespace System.Net.NetworkInformation {
public static class NetworkInformationTimeSpanExtensions {
public static TimeSpan GetPacketReassemblyTimeout(this IPGlobalStatistics statistics);
public static TimeSpan GetMaximumTransmissionTimeout(this TcpStatistics statistics);
public static TimeSpan GetMinimumTransmissionTimeout(this TcpStatistics statistics);
}
}
namespace System.Net.Sockets {
public class Socket {
public TimeSpan ReceiveTimeoutTimeSpan { get; set; }
public TimeSpan ReceiveTimeoutTimeSpan { get; set; }
}
public class TcpClient {
public TimeSpan ReceiveTimeoutTimeSpan { get; set; }
public TimeSpan SendTimeoutTimeSpan { get; set; }
}
}
namespace System.Timers {
public class Timer {
public TimeSpan IntervalTimeSpan { get; set; }
}
}Group 3
namespace System.Data {
// This would need to be a DIM.
public interface IDbCommand {
TimeSpan CommandTimeoutTimeSpan { get; set; }
}
public static class DataTimeSpanExtensions {
public static TimeSpan GetConnectionTimeout(this IDbConnection connection);
}
}
namespace System.Net.Sockets {
public class LingerOption {
public LingerOption(bool enable, TimeSpan time);
public TimeSpan LingerTimeSpan { get; set; }
}
public class Socket {
public void Close(TimeSpan timeout);
}
}Risks
This API does not conflict with other APIs, so I don't see any risk here.
Edit:
Changes to IDbCommand would break implementations (like SqlClient)
/cc @reflectronic
/cc @briangru
/cc @danmoseley
/cc @bartonjs
Reacted by ちーず(･8･)けーき
Activity
deeprobin commentedon Mar 26, 2022
@danmoseley Should I outsource the SqlClient specific stuff to the SqlClient repo or will the API still be discussed in this repo?
[-][API Proposal]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 2/3)[/-][+][API Proposal]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 2+3)[/+]GSPP commentedon Mar 27, 2022
With respect to
SqlConnectionStringBuilder: Does the SQL Server connection string even support non-second timeouts? I believe not, and this would then not be a faithful representation of a connection string. UsingTimeSpanhere would suggest a feature that isn't there, with very rough emulation (rounding to seconds).deeprobin commentedon Mar 27, 2022
@Wraith2 You do quite a lot in the SqlClient. Do you know anything about it?
Wraith2 commentedon Mar 27, 2022
The TDS spec is remarkably cagey about it. Timers are an implementation detail or the client or server and apart from some defaults being in seconds it makes no comment about resolution minimum or maximums.
To the best of my knowledge all current durations in the client library are expressed in seconds but there's no absolute reason that this needs to be the case. I expect that this will continue o be the case.
Adding a timespan wouldn't be harmful and would possibly be an extension of functionality iff client libraries chose to use a higher resolution. I would not expect Microsoft.Data.SqlClient to do that for quite a long time but faster moving libraries like Npgqsl and MySqlConnector to do so if their protocols support higher resolutions. You'd probably want to ask @roji and @bgrainger
roji commentedon Mar 27, 2022
I
don'tdo think there's some value in allowing TimeSpan representations even where non-second timeouts aren't supported: there's no doubt as to the unit (where the meaning of current int properties is completely implicit), and it allows specifying timeouts in larger units, e.g. minutes (though that's not likely to be very useful).Simply adding a DIM IDbCommand.CommandTimeoutTimeSpan doesn't seem very useful - users would need to explicitly cast to IDbCommand in order to make use of it. However, we could also add CommandTimeoutTimeSpan to DbCommand, which is the abstract base class for most ADO.NET command implementations. The default implementation could simply convert the TimeSpan to seconds (and back), and throw if sub-second intervals are specified. Providers which support sub-second intervals would be able to override the property.
What happens with connection string builders (e.g. SqlConnectionStringBuilder) is obviously provider-specific, and each provider can do what's appropriate. If such a property is added, I'd also advise throwing (and not rounding) for unsupported values (e.g. timeouts which don't round to whole second intervals), just like with DbCommand.CommandTimeoutTimeSpan.
Since System.Data.SqlClient is in maintenance-mode, I think any SqlClient-specific additions would probably make sense in Microsoft.Data.SqlClient rather than in System.Data.SqlClient.
/cc @ajcvickers
danmoseley commentedon Mar 27, 2022
@roji can you clarify, I can't tell whether you mean there's value or not based on the rest of what you wrote.
roji commentedon Mar 27, 2022
@danmoseley sorry, there was some editing happening there - I do think there's some value there.
GSPP commentedon Mar 28, 2022
I'm very much for supporting sub-second timeouts (and especially getting rid of the tradition of using integers to represent time - we are not C in 1990). If such configurations are exposed, though, this should go hand in hand with actually supporting it.
I believe that SQL Server timeouts are implemented mostly as client-side timeouts. The client sends an "attention" which is a cancellation request. I'm not sure the server ever times out anything (maybe with Resource Governor or with a concurrent
KILLcommand).Actual support would entail new connection string syntax and internal timer support. If this can't be done then I don't know if the feature is a good idea.
Throwing in case of non-second-integral timeouts seems problematic: Time can involve floating point math and you can end up with a timeout of 2.99999999 seconds. It's also non-discoverable. On the other hand, rounding to the nearest second (or rounding up) can drop a lot of precision for small values, and can be a pit of failure, too. I currently don't see a good way to do this.
If the
TimeSpancapability was added toIDbCommanddirectly, then it would seem like an appropriate trade-off to make. But then,SqlCommandmaybe should implement that interface member explicitly to hide it by default.With respect to the usefulness of sub-second timeouts: I personally never needed it... But I know that some systems are under tight latency constraints. They'd rather fail than be delayed. 1 second can be an eternity for a computer system.
SELECT NULLtakes about 0.25ms on SQL Server. Querying a single small row by primary key from memory takes far below 1ms.Clockwork-Muse commentedon Mar 28, 2022
This is why I loathe the current api that takes
doublefor adding units toTimeSpan(and others) - it absolutely shouldn't be. The number should be understood to be an exact base-10 representation, and it should be parsed as such - it's represented inTimeSpanthat way, after all.The query time is almost never the problem; it's usually network related (either the network itself, or things like serialization of the results).
6 remaining items
ericstj commentedon May 2, 2022
Yes, that sounds like a good plan. The SqlClient API requests can go to https://github.com/dotnet/SqlClient/ as you suggested before.
deeprobin commentedon May 3, 2022
@ericstj See dotnet/SqlClient#1156
dotnet/SqlClient#1602.Can you label this issue with
api-ready-for-review?tarekgh commentedon May 3, 2022
General comment about naming here, I am not a fan of specifying the type
TimeSpanin the property names. Should we use some other term? something likePrecise? so, we can have something likeReadPreciseTimeout?Also, in
System.Data, looks the librarySystem.Data.Commonis targeting the latest .NET versions. I guess we are ok to extend the interfaceIDbCommand. Just wanted to mention that to be conscious about that just in case in the future if decide to support more targeting.tarekgh commentedon May 3, 2022
tagging some more people for awareness: @dotnet/ncl @dotnet/area-system-io @dotnet/area-microsoft-win32 @dotnet/area-system-runtime @ajcvickers @DavoudEshtehari @David-Engel
mattjohnsonpint commentedon May 3, 2022
Question about some of the proposed APIs that are properties with setters. What happens if there's a conflict?
For example:
Which one wins?
tarekgh commentedon May 3, 2022
I guess the last set is the winner. I can see the confusion though when you do
ReceiveTimeoutTimeSpanand try to getReceiveTimeoutafter that, there can be some truncation if we try to do the conversion. The doc would need clarify the behavior.Wraith2 commentedon May 3, 2022
Channelling tanner here, the rounding/truncation method should be specified if possible. It would be confusing if some providers round up and others down or throw.
tarekgh commentedon May 3, 2022
All proposed APIs here are non-virtual. So, the implementation of the new APIs can decide the truncation method and we can document it. The only exception is the
System.Data.IDbCommandthough.ericstj commentedon May 3, 2022
Yes, that's what I would assume. All of these are just a different view over the existing storage. @bartonjs touched on this here #64860 (comment)
bartonjs commentedon May 11, 2023
The general consensus is that the need for two properties (and the TimeSpan suffix) is probably as much of a detractor to the user experience as the benefit of having clear units for the timeout.
Since this is only for a small number of types, and there's not a whole lot of benefit, we think the answer is to just not make the same mistake in the future and let what we have be what we have.
The individual area owners are highly encouraged to make sure that the
<summary>text for each of these properties makes the unit very clear.