Exception types for System.Net.Connections, Part 2

[scalablecory]

We previously approved the following API in #39941:

namespace System.IO
{
    public class NetworkException : IOException
    {
        public static NetworkException Create(NetworkError error, Exception innerException = null, string message = null);
        protected NetworkException(NetworkError error, Exception innerException = null, string message = null);
        // serialization constructor as usual

        public NetworkError NetworkError { get; }
    }

    public enum NetworkError
    {
        Unknown,

        AddressInUse,       // SocketError.AddressAlreadyInUse
        InvalidAddress,     // SocketError.AddressFamilyNotSupported, SocketError.AddressNotAvailable, etc
        ConnectionRefused,  // SocketError.ConnectionRefused
        HostNotFound,       // SocketError.HostNotFound, SocketError.HostUnreachable, etc

        ConnectionAborted,
        ConnectionReset,
    }
}

With the understanding that we would create additional ConnectionResetException, ConnectionAbortedException, etc. and the factory would be updated over time to throw these more derived exceptions as we see fit to add them.

However, there are some changes identified post-review and during implementation that we'd like to discuss with API review:

• @stephentoub, who was unable to attend API review for original issue, strongly believes creating additional derived exception types is a mistake and that we should embrace using exception filters for this.
  • See long discussion at add NetworkException #40344 (comment).
  • Prior guidance from @KrzysztofCwalina and @bartonjs may have been developed before we had exception filters; we should consider updating our stance.
  • If we decide to not do this, we would drop the factory method and flip the protected ctor public.
• @stephentoub called out that exception ctors typically have "message" first. We'd like to reorder and possibly add a second messageless ctor.
• We put the API proposal in the wrong namespace. Moving to System.Net.
• We need to add an OperationAborted error.
• We should consider renaming AddressInUse and InvalidAddress to EndPointInUse and InvalidEndPoint -- "address" comes from the native API struct sockaddr, but our APIs do not actually use this term anywhere.

New proposal with modifications:

namespace System.Net
{
    public class NetworkException : IOException
    {
        public NetworkException(string message, NetworkError error, Exception innerException = null);

        // to enable pulling out standard messages? otherwise make message nullable in above ctor.
        public NetworkException(NetworkError error, Exception innerException = null);

        // serialization constructor as usual

        public NetworkError NetworkError { get; }
    }

    public enum NetworkError
    {
        Unknown,

        EndPointInUse,      // SocketError.AddressAlreadyInUse
        InvalidEndPoint,    // SocketError.AddressFamilyNotSupported, SocketError.AddressNotAvailable
        HostNotFound,       // SocketError.HostNotFound

        ConnectionRefused,  // SocketError.ConnectionRefused
        ConnectionAborted,  // SocketError.ConnectionAborted
        ConnectionReset,    // SocketError.ConnectionReset

        OperationAborted    // SocketError.OperationAborted
    }
}

Old usage

try
{
   throw NetworkException.Create(NetworkError.ConnectionReset);
}
catch(ConnectionResetException ex)
{
   // ...
}
catch(NetworkException ex) when(ex.NetworkError == ConnectionAborted || ex.NetworkError == OperationAborted)
{
  // ...
}

New usage

try
{
   throw new NetworkException(NetworkError.ConnectionReset);
}
catch(NetworkException ex) when(ex.NetworkError == NetworkError.ConnectionReset)
{
   // ...
}
catch(NetworkException ex) when(ex.NetworkError == ConnectionAborted || ex.NetworkError == OperationAborted)
{
  // ...
}

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

[SingleAccretion]

It may be worth noting that with C#9 filtering on enums sometimes becomes somewhat nicer syntactically:

//This:
catch(NetworkException e) when 
    (e.NetworkError == NetworkError.ConnectionAborted || e.NetworkError == NetworkError.OperationAborted)
{ ... }
//Becomes this:
catch(NetworkException e) when 
    (e.NetworkError is NetworkError.ConnectionAborted or NetworkError.OperationAborted)
{ ... }

[geoffkizer]

Frankly, that looks worse to me :)

[SingleAccretion]

To each their own, I guess.