Custom Exceptions should be Serializable

[sir-wilhelm]

We recently ran into an issue where part of our solution crashed after "An unhandled exception terminated the CLR".

This was due to us trying to Serialize the AlreadyClosedException.

We generally treat all Exceptions as Serializable since the Exception type implements ISerializable. and the CA1032 code analysis recommends all custom Exceptions implement 3 constructors (the last of which is a Serialization constructor).

[michaelklishin]

6.0 is a good opportunity to introduce the necessary changes. @sir-wilhelm would you be interested in contributing a PR that does it? Also, are there any downsides to your approach [of treating all exceptions as serializable]?

[sir-wilhelm]

I plan on/should have time to make a PR for it sometime tomorrow.

My thinking is:

• add some test to see if the exceptions are serializable
• make them serializable
• add the constructors CA1032 suggest

I can't think of any downsides or issues this change would cause.

[sir-wilhelm]

I'm waffling on how useful the unit test would be, since it seems like something Code Analysis should cover, namely CA2237: Mark ISerializable types with SerializableAttribute.

I think I'll just add the [Serializable] attribute to RabbitMQClientException and all the Exceptions that extend it.

[sir-wilhelm]

It looks like there was an attempt to add serialization constructors, but they were added commented out in: 4d6d65226403121a2c1357ac750a722c03742f82 when RabbitMQClientException was first created as a base class.

CA2229: Implement serialization constructors recommends adding that constructor for types that implement ISerializable; I think that change could be tracked/fixed in another issue/PR if that change is wanted.

[sir-wilhelm]

ahh, after pulling and looking into it more RabbitMQ.Client.csproj still support netstandard1.5:

<TargetFrameworks>net452;netstandard1.5;netstandard2.0</TargetFrameworks>

So you can't reference the Serialization classes in System.

[michaelklishin]

@sir-wilhelm we can consider bumping it for 6.0. @bording WDYT?

[michaelklishin]

I think the conditional/pragma solution is perfectly acceptable as well. But 6.0 is a good chance to drop netstandard1.5 support if we feel that's the reasonable thing to do in 2020.

[michaelklishin]

@sir-wilhelm I see little value in a unit test for such things but we can add one or two (a serialization roundtrip, maybe?) to sanity check.

[sir-wilhelm]

I don't really see the value in adding a unit test for it either, I think of it being akin to adding a test for a getter of a property on a simple readonly data object.

I think it would be better to set the project/solution up with code analyzers.

If you think the sanity check is worthwhile I can add the test, let me know.

[michaelklishin]

Nah, no need.

[bording]

@sir-wilhelm we can consider bumping it for 6.0. @bording WDYT?

I think it's quite reasonable to drop netstandard1.5 for 6.0.

[lukebakken]

Resolved by #685