-
Notifications
You must be signed in to change notification settings - Fork 10.4k
refactor : use new ThrowIf
overload of ObjectDisposedException
#42613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor : use new ThrowIf
overload of ObjectDisposedException
#42613
Conversation
Thanks for your PR, @ShreyasJejurkar. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
…om/ShreyasJejurkar/aspnetcore into object-disposed-exception-overload
…github.com/ShreyasJejurkar/aspnetcore into object-disposed-exception-overload" This reverts commit a1a0721, reversing changes made to 762397e.
Looks like this change spans multiple areas. |
I think we are good WRT to the components changes since they are very mechanical. |
@Tratcher can you or someone else on the runtime team take a look and make sure you are ok with this change? Feel free to merge if so. /cc: @adityamandaleeka |
@@ -337,7 +332,7 @@ private void ThrowNoReceive() | |||
{ | |||
if (_receiverClosed) | |||
{ | |||
throw new ObjectDisposedException(typeof(TestWebSocket).FullName); | |||
ObjectDisposedException.ThrowIf(_receiverClosed, typeof(TestWebSocket)); |
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.
ObjectDisposedException.ThrowIf(_receiverClosed, typeof(TestWebSocket)); | |
throw new ObjectDisposedException(typeof(TestWebSocket).FullName); |
_receiverClosed is always true here
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.
In the original code, there is an if
condition wrapping exception, that's why I added there to overload. And also in the new overload, we don't have to pass FullName, under the hood it will be retrieved.
See https://source.dot.net/#System.Private.CoreLib/ThrowHelper.cs,393
@@ -337,7 +332,7 @@ private void ThrowNoReceive() | |||
{ | |||
if (_receiverClosed) |
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.
The if/else is no longer needed.
if (_receiverClosed) | |
ObjectDisposedException.ThrowIf(_receiverClosed, typeof(TestWebSocket)); | |
// else _senderClosed | |
throw new IOException("The remote end closed the connection.", new ObjectDisposedException(typeof(TestWebSocket).FullName)); |
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.
Not sure I understand this change! :(
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.
You're doing the if check inside ThrowIf, so you no longer need the outer if/else. If ThrowIf does not throw, it can proceed to the else condition.
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.
I changed it
Approved once the pending comments are addressed. |
LGTM. Thanks |
No functional changes are expected...