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

Fix #46123 - make the subProtocol nullable for HttpListenerContext::AcceptWebSocketAsync #47402

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

euantorano
Copy link
Contributor

Fixes #46123. This makes the subProtocol nullable when accepting web sockets.

A null subProtocol is valid and means no subprotocol, whereas an empty string is not valid so there needs to be a distinction between them.

I hope I've covered everything that needs doing here, but as this is my first contribution please let me know if there's anything I've missed.

@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 Jan 25, 2021

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

Issue Details

Fixes #46123. This makes the subProtocol nullable when accepting web sockets.

A null subProtocol is valid and means no subprotocol, whereas an empty string is not valid so there needs to be a distinction between them.

I hope I've covered everything that needs doing here, but as this is my first contribution please let me know if there's anything I've missed.

Author: euantorano
Assignees: -
Labels:

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

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Jan 25, 2021

CLA assistant check
All CLA requirements met.

@antonfirsov
Copy link
Member

Looks like we even have the tests in HttpListenerContextTests.cs to cover the subProtocol=null cases, it's just the test project has not been updated to use nullable references.

Should be good to merge if #46123 gets the API approval.

@antonfirsov
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub stephentoub merged commit 9b93036 into dotnet:master Jan 26, 2021
@stephentoub
Copy link
Member

@jeffhandley, I don't remember how/where you wanted to track nullability changes...

@jeffhandley
Copy link
Member

@jeffhandley, I don't remember how/where you wanted to track nullability changes...

Here's the running issue: dotnet/docs#21202
Thanks!

@euantorano euantorano deleted the fix/46123 branch January 26, 2021 10:10
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 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.

HttpListenerContext::AcceptWebSocketAsync should have a nullable annotation for the subProtocol
6 participants