-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[API Proposal]: Use TimeSpan everywhere we use an int for seconds, milliseconds, and timeouts (Group 2+3) #67156
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsBackground and motivationAs discussed with @danmoseley this issue serves as a follow up for #14336 and is meant to represent the 2nd group API Proposalnamespace 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; }
}
} RisksThis API does not conflict with other APIs, so I don't see any risk here. /cc @reflectronic
|
@danmoseley Should I outsource the SqlClient specific stuff to the SqlClient repo or will the API still be discussed in this repo? |
With respect to |
@Wraith2 You do quite a lot in the SqlClient. Do you know anything about it? |
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. 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 |
I 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 |
@roji can you clarify, I can't tell whether you mean there's value or not based on the rest of what you wrote. |
@danmoseley sorry, there was some editing happening there - I do think there's some value there. |
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 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 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. |
This is why I loathe the current api that takes
The query time is almost never the problem; it's usually network related (either the network itself, or things like serialization of the results). |
Any additions to Microsoft.Data.SqlClient will take considerable time regardless of difficulty. There is not likely to be a net7.0 build so the earliest this would get in is likely 8 which is 18+ months away. |
That's interesting. I thought shipping faster was one of the key reasons we forked off M.D.SC (along with ability to break API perhaps) |
Being outside runtime makes that possible. Unfortunately small team with a big backlog means that anything new will take time. See the area owner's notes in #51836 (comment) for the basis of my information. |
@ericstj |
Yes, that sounds like a good plan. The SqlClient API requests can go to https://github.com/dotnet/SqlClient/ as you suggested before. |
@ericstj See dotnet/SqlClient#1156 |
General comment about naming here, I am not a fan of specifying the type Also, in |
tagging some more people for awareness: @dotnet/ncl @dotnet/area-system-io @dotnet/area-microsoft-win32 @dotnet/area-system-runtime @ajcvickers @DavoudEshtehari @David-Engel |
Question about some of the proposed APIs that are properties with setters. What happens if there's a conflict? For example: var client = new TcpClient
{
ReceiveTimeout = 5000,
ReceiveTimeoutTimeSpan = TimeSpan.FromMinutes(10)
}; Which one wins? |
I guess the last set is the winner. I can see the confusion though when you do |
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. |
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 |
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) |
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 |
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
Group 3
Risks
This API does not conflict with other APIs, so I don't see any risk here.
Edit:
Changes to
IDbCommand
would break implementations (likeSqlClient
)/cc @reflectronic
/cc @briangru
/cc @danmoseley
/cc @bartonjs
The text was updated successfully, but these errors were encountered: