-
Notifications
You must be signed in to change notification settings - Fork 523
Include address in error message when address already in use (#1086) #1143
Conversation
{ | ||
if ((ex.InnerException as UvException)?.StatusCode == Constants.EADDRINUSE) | ||
{ | ||
throw new IOException($"Failed to bind to address {parsedAddress}: address already in use.", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOException? Would WebListener throw an IOException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Startup is weird, there's really no standard exception you can throw. IO isn't terrible, at least it's related to the socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tratcher Do you know what WebListener would throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just a WebListenerException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's technically a breaking change not to keep this a AggregateException/UvException, but AggregateExceptions are gross enough that I don't think anyone is catching that explicitly. I guess IOException is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is fine. Doesn't seem like a case where someone would catch the specific exception anyway (or at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
{ | ||
if ((ex.InnerException as UvException)?.StatusCode == Constants.EADDRINUSE) | ||
{ | ||
throw new IOException($"Failed to bind to address {parsedAddress}: address already in use.", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is fine. Doesn't seem like a case where someone would catch the specific exception anyway (or at all).
2a7277d
to
2c94884
Compare
#1086
@halter73 @mikeharder