-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Expose setting/getting of system error code #46843
Comments
@elinor-fung I don't think |
Oops, fixed. |
The difference between XXXLastSystemError and XXXLastWin32Error is not indicated by their names, especially if not in Windows. It seems to me that this is our opportunity to remove the Win32 part and create a new Get/Set pair with more explanatory names (Get/SetLastPinvokeError?) with GetLastWin32Error remaining for compatibility. |
The last error pattern is bug-prone. I have seen countless of bugs over the years where the last error is accidentally overwritten. To improve this with source generated interop, we may want to get away with the cached last error concept completely and instead teach the marshalers to include the last system error in the method result. It would address the Win32 name problem as side-effect.
|
Definitely like the idea of getting rid of the existing last error mechanism. So it seems like it would be:
|
Is |
Would eliminating |
Yes, it would have a measurable perf impact for cheap APIs. |
And is that what you're proposing? (I just like to follow along) |
No, I am not proposing to have last error handling to be on by default. My suggestion was to change the opt-in to be via special type used for method return value.
I like it too, but we should watch for the migration issues that it may be introduce. The analyzer/auto-fixer will need to flag all uses of |
Ah - thanks - I missed that bit. |
Multi-targeting would also be an issue, since it wouldn't be a matter of just if-def-ing the We could have a bit of middle ground in terms of how much the analyzer/fixer helps:
Then new types (instead of new namespace System.Runtime.InteropServices
{
/// <summary>
/// Represents the return value and system error for a function
/// </summary>
/// <remarks>
/// When used as the return type for a source-generated P/Invoke (method marked
/// with (<see cref="GeneratedDllImportAttribute"/>), the invoked function is
/// expected to have a return value of <typeparamref name="T"/>. The return value
/// will be stored in <see cref="Value"/> and the system error from after the
/// invocation will be stored in <see cref="Error"/>.
/// </remarks>
/// <typeparam name="T">The type of the value</typeparam>
public readonly struct SystemErrorAndValue<T>
{
public int Error { get; }
public T Value { get; }
public SystemErrorAndValue(int error, T value);
[EditorBrowsable(EditorBrowsableState.Never)]
public void Deconstruct(out int error, out T value);
}
/// <summary>
/// Represents the system error for a function
/// </summary>
/// <remarks>
/// When used as the return type for a source-generated P/Invoke (method marked
/// with (<see cref="GeneratedDllImportAttribute"/>), the invoked function is
/// expected to have a void return. The system error from after the invocation
/// will be stored in <see cref="Error"/>.
/// </remarks>
public readonly struct SystemError
{
public int Error { get; }
public SystemError(int error);
}
} |
Can we change |
Edited to be |
I do not think this is needed at all, every time I do pinvokes I make mine private, and expose an internal version that returns the error code from Now if I needed a value returned from another pinvoke (not MiniDumpWriteDump) then I basically have that as an out param like so: internal static NativeMethods
{
internal static int PInvokeMethod(out IntPtr result, /*args here. */)
{
result = PInvokeMethod(/*args here.*/);
return Marshal.GetLastWin32Error();
}
// mark this with DllImport from the dll it comes from and other stuff.
private static extern IntPtr PInvokeMethod(/*args here.*/);
} And actually I think this is how it is supposed to be done always anyway, I see that the .NET Framework does something like this internally too. |
@AraHaan You are absolutely correct about the usage for the existing |
Updated proposal description to new |
I would omit
|
Good call - updated to move possible new types out of the proposed API. |
One more thought: I think it we should keep |
Added back |
namespace System.Runtime.InteropServices
{
public static class Marshal
{
public static int GetLastSystemError();
public static void SetLastSystemError(int error);
public static int GetLastPInvokeError();
public static void SetLastPInvokeError(int error);
[Obsolete("Use GetLastSystemError() or GetLastPInvokeError()", DiagnosticId="<NextID>", UrlFormat="<the proper URL>")]
public static int GetLastWin32Error();
}
} |
This will be very noisy warning. We have ~500 calls to of Also, should the message just say |
Looking at telemetry, you're right:
The assumption was that this is an advanced API. If we expect this API to be that widely used, obsoleting might be a bit harsh.
I think the reasoning to recommend either was that people didn't necessarily understand that |
It would be fine to recommend |
I got a better idea, for places that use the GetLastWin32Error have ifdefs where if .NET 6+ then use the new api, else use old api for the downlevel stuff that way everyone is happy and then also obsolete the old as well too without too much noise. plus the runtime uses ifdefs anyway on the versions of the TFMS they target anyway so. |
We do use ifdefs in C# as last resort. We strongly prefer code without ifdefs. |
Why would we have to |
There are 200+ projects under src/libraries that target netstandard that cannot use the new API. It is not unusual for these projects to multi-target or share files with projects that target netcoreapp current. So some of the ~500 calls Marshal.GetLastWin32Error would have to stay intact and some of them would have to be under ifdef or disable the obsolete warning. |
Implemented in #51505. |
@elinor-fung would it make sense for you to add a little more to the doc pages for the new methods? Gets the last system error on the current thread It isn't clear which to use when. We also do not have any note in GetLastWin32Error explaining to use other methods in preference when targeting >=.NET 6.0 Worst of all, two of these descriptions don't end with a period |
This statement amused me more than is probably normal. |
Background and Motivation
.NET developers can use
SetLastError
on a P/Invoke to specify whether or not the last error from the invoked function should be stored. The error is then retrievable throughMarshal.GetLastWin32Error
.The runtime's built-in interop marshallers are currently for clearing the system error before invoking the function, getting the system error after invoking the function, and storing the error as the last P/Invoke error code.
With the proposed P/Invoke source generation, a tool (Roslyn source generator) outside of the runtime is responsible for handling the system error such that it can be used/checked by the caller of the P/Invoke.
The proposed APIs would support a source generator's ability to handle this through new APIs for getting/setting the system error and setting the last P/Invoke error.
Proposed API
New APIs to allow getting and setting the system error:
Usage Examples
Example possible usage from a source generator:
Alternative Designs
Get/SetLastSystemError
could be written outside of the runtime (e.g. by invoking into a native function for each platform to match how the runtime gets/sets the system error). Exposing a public API allows developers to get/set the error in a way that is consistent with the runtime on all platforms.Source generators for P/Invokes technically only need to clear the last system error (set it to 0), not set it to any arbitrary value. Providing only a
Get/Clear
(instead ofGet/Set
) seems inconsistent with existing APIs. TheSetLastSystemError
would also give managed functions called from native code the option to explicitly set an error without handling different platforms themselves.The name of
SetLastWin32Error
does not properly reflect the error it would actually set (the last P/Invoke error, not Win32 error), but it is named for consistency with the existingGetLastWin32Error
. An alternative is to create a new pair that is more accurately named:The proposed APIs will be needed for any handling of last system error by a third party. They will allow possible future additions such a specific types for propagating the system error to the caller - for example:
Additions like these can be handled by the third party / source generator, so are not part of this proposal.
Risks
These APIs expose system error code setting that was hidden within the runtime, allowing an easy way for a developer could set the system error. This is already achievable without an API by explicitly P/Invoking into a native function. It would also be possible for a developer to set the last P/Invoke error (call
Marshal.SetLastWin32Error
) outside of a P/Invoke.The text was updated successfully, but these errors were encountered: