Skip to content

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

Merged

Conversation

ShreyasJejurkar
Copy link
Contributor

No functional changes are expected...

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2022
@ghost
Copy link

ghost commented Jul 7, 2022

Thanks for your PR, @ShreyasJejurkar. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jul 7, 2022
@mkArtakMSFT
Copy link
Contributor

Looks like this change spans multiple areas.
@javiercn can you please review the Blazor related areas and @adityamandaleeka you take care of the rest? Thanks!

@javiercn
Copy link
Member

javiercn commented Jul 7, 2022

I think we are good WRT to the components changes since they are very mechanical.

@ShreyasJejurkar ShreyasJejurkar marked this pull request as ready for review July 8, 2022 04:46
@javiercn
Copy link
Member

javiercn commented Jul 14, 2022

@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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ObjectDisposedException.ThrowIf(_receiverClosed, typeof(TestWebSocket));
throw new ObjectDisposedException(typeof(TestWebSocket).FullName);

_receiverClosed is always true here

Copy link
Contributor Author

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)
Copy link
Member

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.

Suggested change
if (_receiverClosed)
ObjectDisposedException.ThrowIf(_receiverClosed, typeof(TestWebSocket));
// else _senderClosed
throw new IOException("The remote end closed the connection.", new ObjectDisposedException(typeof(TestWebSocket).FullName));

Copy link
Contributor Author

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! :(

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it

@Tratcher
Copy link
Member

Approved once the pending comments are addressed.

@JamesNK JamesNK enabled auto-merge (squash) July 16, 2022 00:04
@JamesNK
Copy link
Member

JamesNK commented Jul 16, 2022

LGTM. Thanks

@JamesNK JamesNK merged commit f6362e0 into dotnet:main Jul 16, 2022
@ghost ghost added this to the 7.0-rc1 milestone Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants