Skip to content

Remove QuicStream.ShutdownWriteCompleted method #55981

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
Merged
geoffkizer merged 4 commits into from
Jul 21, 2021
Merged

Remove QuicStream.ShutdownWriteCompleted method #55981

geoffkizer merged 4 commits into from
Jul 21, 2021

Conversation

@geoffkizer
Copy link
Contributor

@geoffkizer geoffkizer commented Jul 20, 2021

No description provided.

@ghost
Copy link

ghost commented Jul 20, 2021

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.

@ghost
Copy link

ghost commented Jul 20, 2021

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

Issue Details
Author: geoffkizer
Assignees: -
Labels:

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

@geoffkizer geoffkizer mentioned this pull request Jul 20, 2021
Closed
CarnaViire
CarnaViire reviewed Jul 20, 2021
View reviewed changes
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs Outdated
return MsQuicStatusCodes.Success;
}

private static uint HandleEventSendShutdownComplete(State state, ref StreamEvent evt)
Copy link
Member

@CarnaViire CarnaViire Jul 20, 2021

Choose a reason for hiding this comment

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

I believe the whole HandleEventSendShutdownComplete can be removed, we already return Success in case of skipped events in HandleEvent

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

Lines 832 to 833 in ca3b279

default:
return MsQuicStatusCodes.Success;

Copy link
Contributor Author

@geoffkizer geoffkizer Jul 21, 2021

Choose a reason for hiding this comment

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

Done

@ghost
Copy link

ghost commented Jul 20, 2021

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

Issue Details
Author: geoffkizer
Assignees: -
Labels:

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

wfurt
wfurt approved these changes Jul 20, 2021
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.

LGTM.
Less code!

@geoffkizer geoffkizer force-pushed the removeshutdownwritescompleted branch from 34fcf38 to 5098faf Compare July 21, 2021 01:04
@geoffkizer
Copy link
Contributor Author

geoffkizer commented Jul 21, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 21, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer geoffkizer merged commit 8ece868 into dotnet:main Jul 21, 2021
@geoffkizer geoffkizer deleted the removeshutdownwritescompleted branch July 21, 2021 22:37
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@JamesNK JamesNK mentioned this pull request Aug 5, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@CarnaViire CarnaViire CarnaViire left review comments

@wfurt wfurt wfurt approved these changes

@ManickaP ManickaP Awaiting requested review from ManickaP

Assignees

No one assigned

Labels

area-System.Net.Quic new-api-needs-documentation

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@geoffkizer @CarnaViire @wfurt @karelz