Add native error codes and messages to managed SNI Named Pipe disconnect errors, so as to match native SNI behavior. - #19135
Conversation
|
|
||
| if (packet.Length == 0) | ||
| { | ||
| return ReportErrorAndReleasePacket(packet, 0, SNICommon.ConnTerminatedError, string.Empty); |
There was a problem hiding this comment.
should you update the identical code in SNITcpHandle.cs?
There was a problem hiding this comment.
@corivera Is this mechanism needed for SNITcpHandle?
| return Interop.Kernel32.GetMessage(error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You shouldn't need these P/Invokes... you can just do:
string message = new Win32Exception(error).Message;Presumably this only happens on error paths where the extra allocation for the exception object won't matter. Win32Exception itself calls either StrError or GetMessage:
https://github.com/dotnet/corefx/blob/master/src/Microsoft.Win32.Primitives/src/System/ComponentModel/Win32Exception.Unix.cs#L9
https://github.com/dotnet/corefx/blob/master/src/Microsoft.Win32.Primitives/src/System/ComponentModel/Win32Exception.Windows.cs#L9
Win32Exception's parameterless ctor also calls Marshal.GetLastWin32Error, so you could skip calling that manually below, too, if you wanted to:
var e = new Win32Exception();
ReportErrorAndReleasePacket(packet, (uint)e.NativeErrorCode, 0, e.Message);
…t errors, so as to match native SNI behavior.
|
Seeing some test failures with Entity Framework tests after adding these changes. Investigating. Edit: Forgot to add return statements for the error codes. Fixed. |
| { | ||
| return ReportErrorAndReleasePacket(packet, 0, SNICommon.ConnTerminatedError, string.Empty); | ||
| var e = new Win32Exception(); | ||
| return ReportErrorAndReleasePacket(packet, (uint)e.NativeErrorCode, 0, e.Message); |
There was a problem hiding this comment.
This may not matter for NP on Linux, but how does Win32Exception work on Linux? For the sake of correctness, do we need to have a platform specific file which references Win32Exception on Windows and something else on Linux?
There was a problem hiding this comment.
Using Win32Exception avoids platform specific files. It has its own versioning for handling error messages and error codes
There was a problem hiding this comment.
I also added native error checking for connection terminated errors in TCP handle, for consistency.
There was a problem hiding this comment.
Ahh. Didn't scroll through completely before commenting.
|
|
||
| if (packet.Length == 0) | ||
| { | ||
| return ReportErrorAndReleasePacket(packet, 0, SNICommon.ConnTerminatedError, string.Empty); |
There was a problem hiding this comment.
@corivera Is this mechanism needed for SNITcpHandle?
6 participants
Resolves some of the issues seen in https://github.com/dotnet/corefx/issues/19057