Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added QuicException.TransportErrorCode #88550

Merged
merged 6 commits into from Jul 17, 2023
Merged

Added QuicException.TransportErrorCode #88550

merged 6 commits into from Jul 17, 2023

Conversation

AlexRadch
Copy link
Contributor

@AlexRadch AlexRadch commented Jul 9, 2023 •
edited by karelz

Resolves #87262 #72666

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 9, 2023
@dotnet-issue-labeler
Copy link

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, 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.

@ghost
Copy link

ghost commented Jul 9, 2023

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

Issue Details

For issue #87262

Author: AlexRadch
Assignees: -
Labels:

new-api-needs-documentation, area-System.Net.Quic
Milestone: -

wfurt
wfurt reviewed Jul 9, 2023
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

please also look at #87679 (comment) and try to address it.

@AlexRadch
Copy link
Contributor Author

please also look at #87679 (comment) and try to address it.

Thank you, I added the transport error code propagation.

@wfurt wfurt mentioned this pull request Jul 9, 2023
Closed
ManickaP
ManickaP reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Looks good modulo comments, thanks!

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicException.cs Show resolved Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Outdated Show resolved Hide resolved
rzikm
rzikm reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ThrowHelper.cs Outdated Show resolved Hide resolved
ManickaP
ManickaP reviewed Jul 14, 2023
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

One last comment, otherwise looks good and we can merge this, thanks!

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ThrowHelper.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ThrowHelper.cs Outdated Show resolved Hide resolved
@karelz karelz added this to the 8.0.0 milestone Jul 16, 2023
@karelz
Copy link
Member

karelz commented Jul 16, 2023

@AlexRadch will you be able to finish the changes? It is highly desirable to merge it by tomorrow 7/17 to catch our Platform complete deadline (= finish all features) for 8.0. Thanks!

@ManickaP
Copy link
Member

I'm taking over, @wfurt has a pending PR on this, we need to get this in today.

This was referenced Jul 17, 2023
Closed
Closed
Closed
Closed
@ManickaP ManickaP merged commit c0971e5 into dotnet:main Jul 17, 2023
99 of 103 checks passed
stephentoub
stephentoub reviewed Jul 17, 2023
View reviewed changes
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs
throw ThrowHelper.GetExceptionForMsQuicStatus(status, $"GetParam({handle}, {parameter}) failed");
}

ThrowHelper.ThrowIfMsQuicError(status, $"GetParam({handle}, {parameter}) failed");
Copy link
Member

Choose a reason for hiding this comment

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

This is now going to allocate a string for the error message even in the success case.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to address this in some of my future PRs. Or @wfurt do you think you could revert these two in #88614?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is ok in interim as it is only perf optimization. I feel it is more important to get the API out.

stephentoub
stephentoub reviewed Jul 17, 2023
View reviewed changes
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs
{
throw ThrowHelper.GetExceptionForMsQuicStatus(status, $"SetParam({handle}, {parameter}) failed");
}
ThrowHelper.ThrowIfMsQuicError(status, $"SetParam({handle}, {parameter}) failed");
Copy link
Member

Choose a reason for hiding this comment

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

This is now going to allocate a string for the error message even in the success case.

@stephentoub stephentoub mentioned this pull request Jul 17, 2023
Merged
@wfurt wfurt mentioned this pull request Jul 18, 2023
Closed
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@ManickaP ManickaP ManickaP approved these changes

@wfurt wfurt wfurt left review comments

@rzikm rzikm rzikm left review comments

Assignees

@ManickaP ManickaP

Labels
area-System.Net.Quic community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add transport error code to QuicException
6 participants
@AlexRadch @karelz @ManickaP @stephentoub @wfurt @rzikm