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.

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

Minor QUIC API changes #55704

Closed
wants to merge 4 commits into from
Closed

Conversation

geoffkizer
Copy link
Contributor

@geoffkizer geoffkizer commented Jul 15, 2021

Remove ShutdownWriteCompleted()
Rename Shutdown() to CompleteWrites()

@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, 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 15, 2021

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

Issue Details

Remove ShutdownWriteComplete()
Rename Shutdown() to CompleteWrites()

Author: geoffkizer
Assignees: -
Labels:

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

Milestone: -

@wfurt
Copy link
Member

wfurt commented Jul 15, 2021

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@wfurt
Copy link
Member

wfurt commented Jul 15, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JamesNK
Copy link
Member

JamesNK commented Jul 15, 2021

While you're changing API, could you add Async suffic to the async methods? e.g. ShutdownCompleted -> ShutdownCompletedAsync.

@wfurt
Copy link
Member

wfurt commented Jul 15, 2021

Shutdown has pretty standard meaning but we may do something different here. SslStream has ShutdownAsync.

The check and azp seems busted ;(

@geoffkizer
Copy link
Contributor Author

The check and azp seems busted ;(

And what a perfect time for it, too :)

@geoffkizer
Copy link
Contributor Author

ShutdownCompleted -> ShutdownCompletedAsync.

We are still discussing whether ShutdownCompleted will go away entirely and be replaced by CloseAsync.

If it doesn't, we can certainly look at renaming this.

@ghost
Copy link

ghost commented Jul 15, 2021

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

Issue Details

Remove ShutdownWriteCompleted()
Rename Shutdown() to CompleteWrites()

Author: geoffkizer
Assignees: -
Labels:

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

Milestone: -

@karelz
Copy link
Member

karelz commented Jul 15, 2021

I thought we decided to push QUIC APIs updates to .NET 7, unless they are must-have for our functional and reliability goals in .NET 6.
@geoffkizer is this one necessary?

We need to keep in mind that while renames are good clean up, it has affect on ASP.NET side as they need to react. Given where we are in the release (almost past platform complete as of today), that may be problematic.
Let's talk about it in today's meeting more ...

@karelz
Copy link
Member

karelz commented Jul 15, 2021

Triage: We are fine with removing the dead code.
The renames are not complete and we will need more in .NET 7, let's wait with them all for .NET 7.

@ManickaP ManickaP mentioned this pull request Jul 16, 2021
@geoffkizer
Copy link
Contributor Author

We need to keep in mind that while renames are good clean up, it has affect on ASP.NET side as they need to react. Given where we are in the release (almost past platform complete as of today), that may be problematic.

I discussed this previously with @JamesNK and he said it was fine to do. In fact as you can see, he's asking for more API renames above :)

@geoffkizer
Copy link
Contributor Author

Okay, I punted the Shutdown rename and reposted this PR here: #55981

@geoffkizer geoffkizer closed this Jul 20, 2021
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants