System.Net.Http.HttpRequestException is not marked as [Serializable]

[magole]

This issue is related to this issue.

Projects like Orleans require types and especially exceptions to be serializable. Most .NET Core exceptions are already marked as such.

The ask is to mark HttpRequestException as [Serializable] and add the requisite protected ctors.

[GrabYourPitchforks]

API proposal:

namespace System.Net.Http
{
    [Serializable] // ADD this attribute
    public class HttpRequestException
    {
        protected HttpRequestException(SerializationInfo info, StreamingContext context); // ADD this ctor
        public override void GetObjectData(SerializationInfo info, StreamingContext context); // OVERRIDE this method
    }
}

This is a request from the Orleans team and matches when we said we'd allow new serialization ctors to be added: (a) it must be an exception-derived type, and (b) the type must exist across both .NET Core and .NET framework. The HttpRequestException type meets both of these.

n.b. Full Framework does not expose the APIs being added here. The HttpRequestException type uses the "partial trust-compliant BinaryFormatter serialization" that was introduced in .NET 4, but that infrastructure does not exist in .NET Core. .NET Core only understands the normal Exception serialization pattern, so that is what's added by this API proposal. There is no plan to add these APIs back to this type in Full Framework. This also means that HttpRequestException instances serialized on Core cannot be deserialized on Full Framework, and vice versa. There is no plan to support this capability.

I'll create a separate issue to track allowing Exception objects to be serialized without using BinaryFormatter or other unsafe-style serialization tech.

[GrabYourPitchforks]

Opened https://github.com/dotnet/designs/issues/158 to track creating safe Exception serialization tech.

[scalablecory]

@dotnet/ncl

[scalablecory]

@GrabYourPitchforks this class is not sealed, adding an interface is a breaking change no?

[GrabYourPitchforks]

It's not a breaking compile-time change. It's potentially a breaking runtime change in that if somebody marked their subclassed type [Serializable] without also implementing ISerializable (which honestly would be very, very weird for an Exception to do), the serialized payload format would now change.

[GrabYourPitchforks]

Oh, derp, of course the base type already implements the interface. I'll update the proposal.

[terrajobst]

Video

• Seems fine per our guidance
• We won't do this for all exception, but only for exceptions that exist in both .NET Framework and .NET Core
• Specifically, we won't do this for .NET Core-only exceptions
• And moving forward, we'll phase out BinaryFormatter anyways

namespace System.Net.Http
{
    [Serializable]
    public class HttpRequestException
    {
        protected HttpRequestException(SerializationInfo info, StreamingContext context);
        public override void GetObjectData(SerializationInfo info, StreamingContext context);
    }
}

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Whoever implements this will need to make sure that the type serializes/deserializes correctly in both directions between .NET Core and .NET Framework, otherwise it'll defeat the purpose of this. And to that end, a) there have been several new members added to HttpRequestException in .NET Core, so they'll need to be properly dealt with, and b) the implementation on .NET Framework uses the "safe serialization" mechanism, and I'm not sure that'll "just work".

[deeprobin]

@stephentoub You're welcome to assign me

[danmoseley]

@GrabYourPitchforks as this missed 6.0, and in 7.0 we expect to disable BinaryFormatter by default and the hypothetical replacement does not depend on ISerializable. is this still relevant?

I realize this package ships on .NET Framework as well, but as noted above, it must deserialize on .NET Core too.

[danmoseley]

@GrabYourPitchforks thoughts?

[danmoseley]

@GrabYourPitchforks ...

[GrabYourPitchforks]

Spoke offline with Steve about this. Since the scenario of cross-serializing between netfx and netcore won't be supported, the "exceptions which exist on both platforms" rule of thumb really doesn't apply, since we really won't be able to support the scenario that rule of thumb was trying to drive at. I'm supportive of just closing this issue as won't-fix.

[danmoseley]

#60600 (comment) mentioned by @bartonjs also describes the cross appdomain scenario. Are we consistent, and is our guidance clear? Or is cross appdomain not relevant here?

[stephentoub]

That isn't relevant here.