Align SqlException Numbers across platforms #3461
Conversation
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniError.netcore.cs
Outdated
Show resolved
Hide resolved
| SqlClientEventSource.Log.TryAdvancedTraceEvent( | ||
| "{0}.{1}{2}Last Socket Exception: {3}", | ||
| nameof(SniTcpHandle), nameof(Connect), EventType.ERR, lastSocketException); | ||
| throw lastSocketException; |
There was a problem hiding this comment.
I'd love to see some tests for these changes.
| if (sniException is SocketException socketException) | ||
| { | ||
| // SocketErrorCode values are cross-plat consistent in .NET (matching native Windows error codes) | ||
| nativeError = (uint)socketException.SocketErrorCode; |
There was a problem hiding this comment.
This is a little concerning, as it will translate SocketError -1 to a positive long (4294967295) and will likely throw an exception here, as there is no unchecked block.
There was a problem hiding this comment.
The same negative int -> uint cast is happening below with Win32Exception too. But in my test program, there is no unchecked exception thrown. The int -1 value happily becomes uint 4294967295. There is no loss of data since both types are 32-bit. I can't tell from this code whether or not the caller expects a large unsigned value for nativeError - none of these public fields are documented. It looks like nativeError ends up in SNIErrorDetails.nativeError, which is also a uint, so I guess that's fine.
Fix logging bug in SqlClientEventSource.
| if (!isConnected) | ||
| { | ||
| // Retrieve the socket error code | ||
| int socketErrorCode = (int)socket.GetSocketOption(SocketOptionLevel.Socket, SocketOptionName.Error); |
There was a problem hiding this comment.
Weird that GetSocketOption() returns object? , but then the docs don't describe when it could be null.
Description
Attempt to better align SqlException Numbers across platforms. Wondering if it might be this simple.
Issues
#1773