This question follows up conversation from PR dotnet/corefx#33024:
https://github.com/dotnet/corefx/pull/33024#discussion_r228693348
According to these links:
1- https://msdn.microsoft.com/en-us/library/windows/desktop/dd877220%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
2- http://man7.org/linux/man-pages/man7/tcp.7.html
The measurements for the TcpKeepAliveIntervaland TcpKeepAliveTimein Windows and Linux OS are different. In Linux the TCP_KEEPINTVL and TCP_KEEPALIVEis set in seconds, while in Windows the SIO_KEEPALIVE_VALSvalues are measured in milliseconds.
In pr dotnet/corefx#33024 we are allowing TCPKeepAlive options on SqlClient code, but we are facing the issue that they would have different values according to the platform.
Would appreciate it if we could get some guidance regarding this.
Related threads:
https://github.com/dotnet/corefx/issues/28337#issuecomment-375752641
https://github.com/dotnet/corefx/pull/29963
Guidance I can't give, opinion I can :grinning:
The other timeouts (Connection, Command) are expressed in integer seconds so it would be consistent to do the same with any new timeout property and then transform it for whichever platform needs it in another scale.
Ignoring constancy it would be "best" to have it typed as a TimeSpan where Linux would only support only second rounded values and windows would offer whatever granularity it can at the ms level.
As highlighted in #33024 TCP_KEEPALIVE / TCP_KEEPIDLE and TCP_KEEPINTVL are expressed in seconds:
The biggest problem is that you cannot get rid of IOControl since it is not feasible for SetSocketOption to abstract it
Well, do we want both interval and time? (and what's the difference?) If we don't need both then it's simple, if we do then I think your original observation that the last one called wins is about all that can be done sensibly.
If as someone observed the SNI value is 30 mins it would seem strange to express that in ms so seconds and having a consistent int typed value seem to be the way to expose it.
what's the difference
Time
The number of seconds a connection will remain idle before the first keep-alive probe is sent
No more used after the connection has been marked to need keep-alive
Interval
The number of seconds a connection will wait for a keep-alive acknowledgement before sending another keepalive probe
do we want both
An API exposing a feature should allow to configure that feature in every aspect
the last one called wins is about all that can be done sensibly
Ok so in your opinion it is correct for a developer consuming the API to have:
// ***** CASE 1
// CODE
socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveTime, TimeSeconds);
socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveInterval, IntervalSeconds);
// EFFECT
// keep-alive time = default
// keep-alive interval = IntervalSeconds
// ***** CASE 2
// CODE
socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveInterval, IntervalSeconds);
socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveTime, TimeSeconds);
// EFFECT
// keep-alive interval = default
// keep-alive time = TimeSeconds
it would seem strange to express that in ms so seconds and having a consistent int typed value seem to be the way to expose it
I reiterate that the current behavior of SetSocketOption is coherent across all operating systems and values are currently in seconds
A software deployed on Windows < Win 10 v1709 is not able to perform SetSocketOption system calls since that system API is not existent: in those cases socket.IOControl with milliseconds values should be called and there is no smart way to convert a call setting one parameter in a call setting two paramenters without undesirable/undesired behavior
there is no smart way to convert a call setting one parameter in a call setting two paramenters without undesirable/undesired behavior
There's no way to read the currently set values from the OS? If you could read it, then you could read the current values, replace one with the value to be overwritten, and write them back. That doesn't work?
There are also other options. For example, on older Windows, we could store the values from previous calls for these settings, and then use all of them on subsequent calls. We could use a conditional weak table to store the additional state. Then it would be pay for play, only incurring additional overhead when using these new settings on an older OS.
Thanks for your reply @stephentoub, it demonstrates that collaboration generates better solutions and help developers grow, learning from more skilled professionals
There's no way to read the currently set values from the OS?
Unfortunately it is not possible 馃槥
I assume your idea is using as default values:
on older Windows, we could store the values from previous calls for these settings
We could declare the two integers as internal in Socket.Windows and use them from SocketPal.Windows when:
SocketOptionLevel.TcpSocketOptionName.TcpKeepAliveInterval or SocketOptionName.TcpKeepAliveTimewe could use a conditional weak table to store the additional state
ConditionalWeakTable accepts only reference types, therefore I imagine having something like ConditionalWeakTable<Socket, ClassContainingTimeAndInterval>
Do you meant having a ConditionalWeakTable for each socket or a shared one?
In the first case we could use Socket.Windows, whereas in the second case SocketPal.Windows could be suitable, but, if we declare a lot of sockets, the table can grow and impact performances
I think it would be better to open an issue that references https://github.com/dotnet/corefx/issues/25040 to track this API normalization need.
Should a PR addressing it deprecate/remove IOControl not being cross-platform?
to track this API normalization need.
That's what this issue is, no?
If you mean an issue for additional members, I don't think we should add additional APIs for this. The recently added surface area covers the latest versions of all supported OSes, yes? I don't believe we should be adding additional APIs to support older OSes. Let's make the newly added ones instead work as best as possible.
Do you meant having a ConditionalWeakTable for each socket or a shared one?
Shared. That's the main purpose of a CWT, to add additional "members" to instances that are the keys.
if we declare a lot of sockets, the table can grow and impact performances
Only on older Windows and only if you use these new APIs. At which point if you have a perf issue you can upgrade your OS (or not use the new members). I think that's fine. I also expect perf will be acceptable, regardless.
The other option is to just make sure these new APIs don't throw when used on older OSes, making them usable anywhere and just nops when not supported.
That's what this issue is, no?
Sure, my fault
The other option is to just make sure these new APIs don't throw when used on older OSes, making them usable anywhere and just nops when not supported.
If I remember well, the decision was to throw as done for other cases in which options where not supported
Not throwing will not allow us to have the same API working the same way across platforms, but
considering that with time people are moving to new OSes it could be a valuable choice
On the other hand, if we implement SetSocketOption to call IOControl with the tricks you suggested we won't have not supported exceptions, but we will have code to remove in the future when those OS versions will be no more supported by .NET
Who can decide which solution is better?
with the tricks you suggested we won't have not supported exceptions, but we will have code to remove in the future when those OS versions will be no more supported by .NET.Who can decide which solution is better?
Let's try and validate the CWT solution. If it works as expected, let's go with that unless/until we find something better. Sound good?
Thanks for your help.
Ok, thank you!
Since we are trying to provide a uniform experience to API consumers, should we, in this or another issue, address also the keep-alive retry count and how?
It is the number of unacknowledged keep-alive probes to send before considering the connection dead and terminating it
Windows 2000, Windows XP and Windows Server 2003
Windows Vista and later < Windows 10 V 1703
Windows 10 >= Windows 10 V 1703
Linux and OSX
should we, in this or another issue, address also the keep-alive retry count and how?
The current state here seems reasonable, so I don't think there's more that needs to be done. On all supported OSes where we it can be changed (Win2K, XP, 2K3, and Vista are not supported by .NET Core), you control the value via SetSocketOption, and on Win7/Win8 where it can't be changed anyway, you'll get an exception that could be caught and ignored if desired. The default values on Windows/Linux/macOS are all reasonably close, but even if they weren't, there are lots of places where operating systems have different defaults for things, and that's one of those OS details we don't need to unify.
Ok perfect!
Just to summarize the behavior on Windows:
Most helpful comment
There's no way to read the currently set values from the OS? If you could read it, then you could read the current values, replace one with the value to be overwritten, and write them back. That doesn't work?
There are also other options. For example, on older Windows, we could store the values from previous calls for these settings, and then use all of them on subsequent calls. We could use a conditional weak table to store the additional state. Then it would be pay for play, only incurring additional overhead when using these new settings on an older OS.