Skip to content
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

Exception.HResult setter should be made public #26163

Closed
luqunl opened this issue May 14, 2018 · 1 comment
Closed

Exception.HResult setter should be made public #26163

luqunl opened this issue May 14, 2018 · 1 comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@luqunl
Copy link
Contributor

luqunl commented May 14, 2018

Currently The setter on Exception.HResult is protected. If customer need to set a different HResult for a specific exception, customer has to create a derived exception to override.

Rationale and Usage

It is very common to set a different HResult for interop area due to compatibility reason --- Interop should return same HResult as the code is called/implemented by native code.

For example:

https://github.com/dotnet/corefx/blob/02a26f8f417967e31bab8d96605d3d8846154e8a/src/System.Runtime.WindowsRuntime/src/System/IO/NetFxToWinRtStreamAdapter.cs#L195

Exception.SetErrorCode is an internal API. The HResult for ObjectDisposedException is 0x80131622 and RO_E_CLOSED is 0x80000013

        if (_fClosed)
        {
            return RO_E_CLOSED;
        }

With this change, you can directly assign HResult and remove SetErrorCode internal API.

ex. HResult  = __HResults.RO_E_CLOSED;

Proposed API

    public partial class Exception : System.Runtime.Serialization.ISerializable
    {
        // Existing members
        // public int HResult { get { throw null; } protected set { } }
         // Proposed members
         public int HResult { get { throw null; } set { } }
    }

Details

There are other ways to achieve this:

  • Change Exception.SetErrorCode from Internal to public API 
    Pro: a lot of existing code already use this API, less code change
    Con:  More complicated than proposed one. There are two ways to change HResult(Derived from exists exception and use Exception.SetErrorCode API). 
    
  • Add a new public API in Marshal which update HResult for an exception
    Pro: It works
    Con: Complicated and customer have to know this new Marshal API exists.
    

Overall, Make Exception.HResult setter to public is simpler way to do this.

@danmoseley
Copy link
Member

@luqunl can this be closed?

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

No branches or pull requests

3 participants