add NetworkException #40344

geoffkizer · 2020-08-04T22:46:36Z

Contributes to #39941

Feedback on error messages would be appreciated... I mostly stole these from the SocketException messages, but those aren't all that clear to begin with.

@scalablecory @stephentoub @halter73 @davidfowl @dotnet/ncl

Dotnet-GitSync-Bot · 2020-08-04T22:46:38Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Dotnet-GitSync-Bot · 2020-08-04T22:46:40Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2020-08-04T22:47:21Z

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

stephentoub · 2020-08-04T22:53:09Z

src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs

+        protected NetworkException(NetworkError error, Exception? innerException = null, string? message = null) { }
+        protected NetworkException(System.Runtime.Serialization.SerializationInfo serializationInfo, System.Runtime.Serialization.StreamingContext streamingContext) { }
+        public NetworkError NetworkError { get { throw null; } }
+        public static NetworkException Create(NetworkError error, Exception? innerException = null, string? message = null) { throw null; }


I wasn't in the API review. Why does this use protected ctors and a factory method? That's very uncommon with exceptions.

The idea is that we could have derived exceptions like AddressInUseException later on, and that Create would be updated to throw those when we later add them.

Why would we add such types? What additional information would the exception carry that would require a custom type?

I think it's pretty unlikely we would ever add such types. The discussion in the meeting went around a bit and this may be an artifact of some intermediate stage of dicussion.

I'd be happy to just remove the factory method on the assumption that we'll never add those types. As you say, there doesn't really seem to be any reason to add them, since you already have all the info you need.

Guided by @bartonjs guidance that exception filters should not be the primary method of distinguishing exceptions that you would take different actions on.

I'll give an argument for having distinct exception types:

We are no longer giving OS error codes with this API. We have distilled/merged similar error codes together, to the point where it really doesn't seem like someone would want to take action on multiple of them in a single catch block -- the actions would be so different that multiple catch blocks are warranted.

If that is true, then having multiple exception types makes more sense and would bring us a little closer to what I view as idiomatic C#, while also providing an error code leaves an escape hatch for anyone who would be in the rare situation of needing to combine multiple error codes.

So you're saying the code that ASP.NET has (https://source.dot.net/#Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets/Internal/SocketConnection.cs,144) that currently uses an exception filter would stop doing so? I don't buy it, especially when factoring in that it's not only looking at multiple exception types, but multiple error codes conditioned by platform.

@stephentoub we should consider using that logic here so that they don't have to. That should be a goal when reducing the SocketError into NetworkError.

To me, it's about making sure that people can write their handlers correctly.

If all errors are ignored, there's no catch, everything works.

If all errors are retried, there's no filter, and the one exception type (which would be the base class for the specially singled-out ones) is sufficient.

If a "common" (to whatever extent you want to call something common) pattern is to only care about (e.g.) ConnectionReset, then a distinct type has a better chance of producing code to match that desire than a filter.

If you leave off the filter, it still catches when you wanted, but also when you didn't.

If ConnectionReset ever gets more granular, you need to update your filter, but have no good way of knowing it, which makes an extensible-looking model be actually brittle.

In the discussion, it was suggested that ConnectionReset, ConnectionAborted, and possibly AddressInUse were the most likely things to be handled in a manner other than "any connection error". Leading to the hybrid model:

NetworkException : IOException

static NetworkException Create(NetworkError)

enum NetworkError

ConnectionResetException : NetworkException

ConnectionAbortedException : NetworkException

AddressInUseException : NetworkException

InvalidAddress is mostly a "something weird made it this far" (IIUC), so it seems (to me, and as suggested in the meeting) to not be something that would ever warrant a specialty type... because no one would really be handling it as a special case. And while the other errors do have their particular uses (largely in painting UI in special ways for error handling) they might be handled rarely enough that they also aren't worth specialty types.

SocketError, for example, has over 50 members; in no world would I want us adding over 50 exception type

While I'm not a sockets guru, I don't expect that there's really any recovery action for NotSocket; so it wouldn't warrant a specialty type (no special handler, no special type). The fact that NotSocket happened is definitely useful for logging, so reporting the information is useful. And maybe there's a super-power-scenario where someone can do something unique and clever, so also being programmatic isn't harmful.

Based on the suggested mappings for NetworkError from SocketError, many of the SocketError values are "similar, but different in nuance", and those would probably benefit/have-benefitted from a tiny bit of hierarchy, like UnsupportedProtocolException : SocketException, to represent ProtocolType/ProtocolOption/ProtocolNotSupported (I'm just guessing based on names).

I don't think the position follows from those CONSIDER/DO NOTs

The conclusion that I draw from them is "everything that should be handled the same way in common usage of the API should be the same exception type", and "everything that should be handled in distinct ways in common usage of the API should be different exception types"; again, using robustness and the ability to evolve as the justification. While Krzysztof's comment sort of suggests never having the programmatic properties, I don't quite get that from the guidelines. I think that the hybrid model is a nice balance for "make easy things easy and hard things possible", or a variant "make common needs easy to fulfill, and uncommon needs possible to fulfill".

The impression I got from the meeting is ConnectionAborted and ConnectionReset are "commonly" handled, and are handled differently from each other, so different exception types are warranted. The fact that the referenced ASP.Net code defined exceptions of exactly those names feels like reinforcement of that belief.

we should consider using that logic here so that they don't have to. That should be a goal when reducing the SocketError into NetworkError.

ASP.NET can choose to make such policy decisions as to how it reacts to various conditions. I don't think the same holds for the library. The comment there indicates it's only sometimes returning a different code than expected; we'd be applying a broken policy if we then always did such a translation for other consumers.

idiomatic C#,

And the next catch block that handles both an abort and a dispose in the same catch block? That's not "idiomatic C#"? It sure seems to me like a completely valid, prototypical use. If what you mean by idiomatic is "wouldn't have been written before C# 7", then sure, and we may as well stop investing in the language if we say anything beyond the initial releases isn't recommended.

I honestly don't understand the argument here for derived types. If two NetworkExceptions differ only in error code, and carry no different data otherwise, I see no good reason to introduce yet another exception type. It provides zero additional benefit from my perspective, only complication.

And with a design like the one approved, nothing prevents two different derived types from using the same error code. At that point, I can't handle it just by derived type and have to use the base type with error code to be correct.

src/libraries/System.Net.Primitives/src/System/Net/NetworkException.cs

src/libraries/System.Net.Primitives/src/System/Net/NetworkError.cs

stephentoub · 2020-08-04T22:58:32Z

src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs

+    }
+    public class NetworkException : System.IO.IOException
+    {
+        protected NetworkException(NetworkError error, Exception? innerException = null, string? message = null) { }


This order is strange. message (almost?) always comes before innerException.

They're ordered from most used to least used.

That may be, but where else in the dozens or hundreds of exception types in .NET does this order occur? It's very inconsistent. @bartonjs, you're ok with this?

The order from the ctor ended up just being what we designed the factory method as; which was based on the notion that it would be throw NetworkException.Create(NetworkError.ConnectionAborted);, which would allow the easy creation of a derived type later, to avoid using exception filters as the primary means of distinguishing errors that seem catchable.

I don't think we really spent a lot of time and thinking on this line, TBH; and @geoffkizer was pushing for a fast "can we just approve it and move on", which we apparently did 😄.

@geoffkizer was pushing for a fast "can we just approve it and move on"

I plead guilty to this :)

By far the most common usage here is, give an error code and an optional inner exception. It's pretty weird to want to customize the error message. But I don't think we should rule it out entirely -- especially for the NetworkError.Unknown case, it's useful for the provider to be able to provide more info in the exception message.

Perhaps we could finesse this by having two overloads:

NetworkException(NetworkError error, Exception? innerException = null)

NetworkException(string message, NetworkError error, Exception? innerException = null)

src/libraries/System.Net.Primitives/src/Resources/Strings.resx

Co-authored-by: Cory Nelson <[email protected]>

geoffkizer · 2020-08-05T02:16:19Z

I added OperationAborted as well, and fixed up the other issues above.

We need to decide on:
(1) Factory method or no
(2) Order of parameters on factory method/constructor

geoffkizer · 2020-08-05T03:06:52Z

I also tried to improve the exception messages. Please take a look.

geoffkizer · 2020-08-05T03:22:47Z

Pushed a change so that SocketsHttpConnectionFactory will now throw NetworkExceptions.

stephentoub · 2020-08-05T11:16:08Z

Pushed a change so that SocketsHttpConnectionFactory will now throw NetworkExceptions.

I assume use in NetworkStream will come subsequently?

stephentoub · 2020-08-05T11:19:12Z

src/libraries/System.Net.Primitives/tests/FunctionalTests/NetworkExceptionTest.cs

+        [Fact]
+        public static void Create_InnerExceptionAndMessage_Success()
+        {
+            const string message = "Hello";


Nit: message => Message

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Connections/SocketConnection.cs

scalablecory · 2020-08-05T17:45:00Z

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpConnectionFactory.cs

+            NetworkError error = socketException.SocketErrorCode switch
+            {
+                SocketError.AddressAlreadyInUse => NetworkError.AddressInUse,
+                SocketError.AddressFamilyNotSupported => NetworkError.InvalidAddress,
+                SocketError.AddressNotAvailable => NetworkError.InvalidAddress,
+                SocketError.HostNotFound => NetworkError.HostNotFound,
+                SocketError.ConnectionRefused => NetworkError.ConnectionRefused,
+                SocketError.OperationAborted => NetworkError.OperationAborted,
+                SocketError.ConnectionAborted => NetworkError.ConnectionAborted,
+                SocketError.ConnectionReset => NetworkError.ConnectionReset,
+                _ => NetworkError.Unknown
+            };


It probably doesn't matter, but code size can be reduced by using a switch statement rather than a switch expression, by having the NetworkError.InvalidAddress cases fall through to a single branch.

I'm kind of surprised that switch expressions don't support this sort of thing...

…andler/Connections/SocketConnection.cs add ExceptionDispatchInfo handling Co-authored-by: Cory Nelson <[email protected]>

geoffkizer · 2020-08-06T07:40:11Z

I assume use in NetworkStream will come subsequently?

Yes.

geoffkizer · 2020-08-06T07:45:17Z

Since this implements the API as approved, and since there doesn't seem to be any consensus to change it, I'm planning to merge as is when CI completes, in the interest of getting this in sooner rather than later.

@stephentoub I agree with your points, and I'd be happy to raise these in a subsequent API review if you'd like me to.

src/libraries/System.Net.Primitives/src/Resources/Strings.resx

address -> EndPoint in error messages Co-authored-by: Cory Nelson <[email protected]>

src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs

geoffkizer · 2020-08-06T14:48:16Z

Pushed a change to make NetworkStream always throw NetworkException instead of IOException.

Note, there's some questionable exception handling in NetworkStream currently -- specifically, lots of cases where it throws NetworkException (previously IOException), either with a wrapped inner exception or not, where it could arguably be throwing a different exception type. Examples include:

(1) Constructor throws NetworkException if the underlying socket isn't appropriate (e.g. UDP). This could arguably be ArgumentException or InvalidOperationException.
(2) All the read/write methods (incl async) wrap any exception they receive from the socket APIs (except, oddly, OutOfMemory). This includes ObjectDisposedException when the underlying Socket is disposed. It also includes passing an invalid IAsyncResult to EndRead/Write, which generates an ArgumentException that is then wrapped. It seems like only wrapping SocketException would be better.

If there's consensus to change these exception cases, I'll happily make the change.

geoffkizer · 2020-08-06T15:06:05Z

Also pushed a change to remove SocketConnectionNetworkStream per discussion with @scalablecory.

scalablecory · 2020-08-06T15:20:33Z

(1) Constructor throws NetworkException if the underlying socket isn't appropriate (e.g. UDP). This could arguably be ArgumentException or InvalidOperationException.
(2) All the read/write methods (incl async) wrap any exception they receive from the socket APIs (except, oddly, OutOfMemory). This includes ObjectDisposedException when the underlying Socket is disposed. It also includes passing an invalid IAsyncResult to EndRead/Write, which generates an ArgumentException that is then wrapped. It seems like only wrapping SocketException would be better.

These would be breaking changes, though maybe some of them would be acceptable. I wouldn't do anything in this PR though.

geoffkizer · 2020-08-06T15:25:35Z

These would be breaking changes, though maybe some of them would be acceptable.

Agreed, though most of these cases are downright weird, so I suspect changing them is reasonable. Not super important though.

I wouldn't do anything in this PR though.

The only reason to consider doing something in this PR is that all of these non-SocketException cases result in a NetworkException with NetworkError=Unknown. That's a bit confusing. I think we really don't want to add an enum value in NetworkError for something like "you tried to create a NetworkStream on a UDP socket". But that means either live with NetworkError=Unknown, which is probably fine, or change this to some non-NetworkException thing.

geoffkizer · 2020-08-06T15:27:01Z

Or maybe change NetworkError.Unknown to NetworkError.Other.

These cases are super weird and I don't think it's worth spending much time worrying about them.

antonfirsov · 2020-08-06T17:57:50Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs

+                    Assert.Throws<NetworkException>(() => server.Read(new byte[1], 0, 1));
+                    Assert.Throws<NetworkException>(() => server.Write(new byte[1], 0, 1));

-                    Assert.Throws<IOException>(() => server.BeginRead(new byte[1], 0, 1, null, null));
-                    Assert.Throws<IOException>(() => server.BeginWrite(new byte[1], 0, 1, null, null));
+                    Assert.Throws<NetworkException>(() => server.BeginRead(new byte[1], 0, 1, null, null));
+                    Assert.Throws<NetworkException>(() => server.BeginWrite(new byte[1], 0, 1, null, null));

-                    Assert.Throws<IOException>(() => { server.ReadAsync(new byte[1], 0, 1); });
-                    Assert.Throws<IOException>(() => { server.WriteAsync(new byte[1], 0, 1); });
+                    Assert.Throws<NetworkException>(() => { server.ReadAsync(new byte[1], 0, 1); });
+                    Assert.Throws<NetworkException>(() => { server.WriteAsync(new byte[1], 0, 1); });


I know it's a deficiency of the original tests, but shouldn't we add assertions for exception.NetworkError with this refactor?

We could, but these are just wrapping ObjectDisposedException so the NetworkError would be Unknown, which isn't all that useful to validate.

geoffkizer · 2020-08-06T19:04:25Z

I pushed a change with the latest updates from API review. This is ready to merge, please take a look.

src/libraries/System.Net.Primitives/src/System/Net/NetworkException.cs

…ption.cs Co-authored-by: Cory Nelson <[email protected]>

scalablecory

lgtm

antonfirsov · 2020-08-06T22:29:51Z

Isn't the replacement of all exceptions thrown by NetworkStream a breaking change of an unacceptable volume? I imagine we may break way too many customers with this, or even stuff within dotnet/* repositories.

I think we should at least add the breaking label here as a minimum.

scalablecory · 2020-08-06T22:50:56Z

Isn't the replacement of all exceptions thrown by NetworkStream a breaking change of an unacceptable volume? I imagine we may break way too many customers with this, or even stuff within dotnet/* repositories.

I think we should at least add the breaking label here as a minimum.

We are only throwing a more-derived exception, nothing here should be breaking.

* add NetworkException

antonfirsov · 2020-08-13T15:16:30Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs

                {
                    serverSocket.Dispose();

-                    Assert.Throws<IOException>(() => server.Read(new byte[1], 0, 1));


Just marking this diff sorry.

add NetworkException

9b84cf7

Dotnet-GitSync-Bot added the new-api-needs-documentation label Aug 4, 2020

geoffkizer added the area-System.Net label Aug 4, 2020

geoffkizer added this to the 5.0.0 milestone Aug 4, 2020

stephentoub reviewed Aug 4, 2020

View reviewed changes

scalablecory reviewed Aug 4, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/Resources/Strings.resx Outdated Show resolved Hide resolved

scalablecory reviewed Aug 4, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/Resources/Strings.resx Outdated Show resolved Hide resolved

geoffkizer and others added 3 commits August 4, 2020 17:20

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx

5b5779d

Co-authored-by: Cory Nelson <[email protected]>

add proper serialization logic

78b6b4d

add OperationAborted and better XML comments and error strings

6b83c4e

use NetworkException in SocketsHttpConnectionFactory

298f96d

stephentoub reviewed Aug 5, 2020

View reviewed changes

scalablecory reviewed Aug 5, 2020

View reviewed changes

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Connections/SocketConnection.cs Outdated Show resolved Hide resolved

scalablecory reviewed Aug 5, 2020

View reviewed changes

geoffkizer and others added 2 commits August 6, 2020 00:36

Update src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpH…

bffcabb

…andler/Connections/SocketConnection.cs add ExceptionDispatchInfo handling Co-authored-by: Cory Nelson <[email protected]>

fix casing of constant in test

b3cfa89

scalablecory reviewed Aug 6, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/Resources/Strings.resx Outdated Show resolved Hide resolved

scalablecory reviewed Aug 6, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/Resources/Strings.resx Outdated Show resolved Hide resolved

geoffkizer and others added 2 commits August 6, 2020 00:50

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx

563f86a

address -> EndPoint in error messages Co-authored-by: Cory Nelson <[email protected]>

Update src/libraries/System.Net.Primitives/src/Resources/Strings.resx

11fc432

address -> EndPoint in error messages Co-authored-by: Cory Nelson <[email protected]>

scalablecory reviewed Aug 6, 2020

View reviewed changes

src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs Outdated Show resolved Hide resolved

Address -> EndPoint in enum values

1f8d94b

scalablecory mentioned this pull request Aug 6, 2020

Exception types for System.Net.Connections, Part 2 #40451

Closed

Geoffrey Kizer added 2 commits August 6, 2020 06:46

move MapSocketError to a common file and fix some review issues

6d7872a

update NetworkStream to use NetworkException

2185ee5

remove SocketConnectionNetworkStream and use NetworkStream directly

a120d07

antonfirsov reviewed Aug 6, 2020

View reviewed changes

Geoffrey Kizer added 2 commits August 6, 2020 11:49

remove InvalidEndPoint

7785efe

remove factory method and update constructor arguments

9a17607

scalablecory reviewed Aug 6, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/System/Net/NetworkException.cs Outdated Show resolved Hide resolved

scalablecory reviewed Aug 6, 2020

View reviewed changes

src/libraries/System.Net.Primitives/src/System/Net/NetworkException.cs Show resolved Hide resolved

Update src/libraries/System.Net.Primitives/src/System/Net/NetworkExce…

db769f5

…ption.cs Co-authored-by: Cory Nelson <[email protected]>

scalablecory approved these changes Aug 6, 2020

View reviewed changes

add OperationAborted to GetExceptionMessage

c266425

antonfirsov mentioned this pull request Aug 7, 2020

Replace SocketsHttpConnectionFactory with SocketsConnectionFactory #40506

Merged

geoffkizer merged commit 4b59394 into dotnet:master Aug 7, 2020

geoffkizer mentioned this pull request Aug 7, 2020

Exception types for System.Net.Connections #39941

Closed

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020

add NetworkException (dotnet#40344)

58c3f4b

* add NetworkException

antonfirsov mentioned this pull request Aug 11, 2020

Test failure: System.Net.Tests.HttpWebRequestTest_Async.GetResponseAsync_ServerNameNotInDns_ThrowsWebException #40606

Closed

antonfirsov reviewed Aug 13, 2020

View reviewed changes

geoffkizer deleted the networkexception branch November 7, 2020 23:41

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

add NetworkException #40344

add NetworkException #40344

Uh oh!

Conversation

geoffkizer commented Aug 4, 2020

Uh oh!

Dotnet-GitSync-Bot commented Aug 4, 2020

Uh oh!

Dotnet-GitSync-Bot commented Aug 4, 2020

Uh oh!

ghost commented Aug 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scalablecory Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Aug 5, 2020

Uh oh!

geoffkizer commented Aug 5, 2020

Uh oh!

geoffkizer commented Aug 5, 2020

Uh oh!

stephentoub commented Aug 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Aug 6, 2020

Uh oh!

geoffkizer commented Aug 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Aug 6, 2020

Uh oh!

geoffkizer commented Aug 6, 2020

scalablecory Aug 5, 2020 •

edited

Loading

stephentoub Aug 5, 2020 •

edited

Loading

stephentoub Aug 5, 2020 •

edited

Loading

antonfirsov Aug 6, 2020 •

edited

Loading

antonfirsov commented Aug 6, 2020 •

edited

Loading