KEMBAR78
Utilize new APIs in the WebSocket implementation by PaulusParssinen · Pull Request #101953 · dotnet/runtime · GitHub
Skip to content

Utilize new APIs in the WebSocket implementation #101953

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
merged 19 commits into from
May 21, 2024
Merged

Utilize new APIs in the WebSocket implementation #101953

merged 19 commits into from
May 21, 2024

Conversation

@PaulusParssinen
Copy link
Contributor

Use few newer APIs in the managed websocket implementation. Gets rid of little manual endianness bit-twiddling that is common for code before BinaryPrimitives.

@ghost ghost added the area-System.Net label May 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2024
@dotnet-policy-service
Copy link
Contributor

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

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented May 8, 2024 •
edited
Loading

Assuming the CI failures are unrelated but the fails were about missing log artifact directory, so I'm rolling the CI roulette one more time.

edit: hooray, finally green

@CarnaViire
Copy link
Member

@zlatanov JFYI

CarnaViire
CarnaViire reviewed May 9, 2024
View reviewed changes
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
sendBuffer[i] = unchecked((byte)length);
length /= 256;
}
BinaryPrimitives.WriteUInt64BigEndian(sendBuffer.AsSpan(2), (ulong)payload.Length);
Copy link
Member

Choose a reason for hiding this comment

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

We do already have a test that would verify it, is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not explicitly, but the deflate / inflate tests would hit this and verify its correctness.

Copy link
Contributor Author

@PaulusParssinen PaulusParssinen May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The testing coverage is dangerously low but a lot of things would should have broke if these were wrong, for example the compression tests as said above😅

@PaulusParssinen
Use BinaryPrimitives in more places
2868bd6
@PaulusParssinen
Restore the behavior where mask is stored in machine-endianness
505f111 
* Implementation detail, both would work
@PaulusParssinen
Remove unused using directives
820dc04
@PaulusParssinen
Read the payload length as unsigned for 16-bit case
79ab34b 
RFC6455, Section 5.2. quote: "If 126, the following 2 bytes interpreted as a 16-bit unsigned integer are the payload length."
@PaulusParssinen PaulusParssinen marked this pull request as draft May 9, 2024 20:01
@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented May 9, 2024 •
edited
Loading

I just realized that WebSocketValidate.cs is in shared folder, but.. it's not actually imported by any of the projects in the class and only other place it's "synced" to is azure-relay-dotnet?

https://grep.app/search?q=websocketvalidate&filter[lang][0]=C%23&filter[path][0]=src/&filter[repo][0]=Azure/azure-relay-dotnet

Can we move the class to System.Net.WebSockets?

edit: Answer is no. (It is used by couple projects. Need to restore the removed method.)

@PaulusParssinen PaulusParssinen marked this pull request as ready for review May 9, 2024 21:27
@zlatanov
Copy link
Contributor

LGTM. Thanks!

CarnaViire
CarnaViire approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! (and sorry for the delay)

@CarnaViire CarnaViire merged commit e8b89a3 into dotnet:main May 21, 2024
@PaulusParssinen PaulusParssinen deleted the use-new-apis-in-ws branch May 22, 2024 09:13
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@PaulusParssinen @Ruihan-Yin
Utilize new APIs in the WebSocket implementation (dotnet#101953)
3e2ec7b 
* Use BinaryPrimitives in ManagedWebSocket

* Use MemoryMarshal.CreateSpan instead of .ctor in WebSocketInflater

* Apply PR suggestion

* Use BinaryPrimitives in more places

* Restore the behavior where mask is stored in machine-endianness

* Implementation detail, both would work

* Remove unused using directives

* Read the payload length as unsigned for 16-bit case

RFC6455, Section 5.2. quote: "If 126, the following 2 bytes interpreted as a 16-bit unsigned integer are the payload length."

* Consolidate some validation logic to WebSocketValidata.cs

* Use CallerArgumentExpression to flow the parameterName in ValidateArraySegment

* Remove unused ValidateBuffer

* Use new throw helpers in ValueWebSocketReceiveResult

* Use ValueTask.FromException

* Revert "Remove unused ValidateBuffer"

This reverts commit 053f0ac.

* Oops

* Fix ArgumentNullException for ValidateArraySegment

* Revert "Consolidate some validation logic to WebSocketValidata.cs"

This reverts commit c515ba9.

* Revert ValueWebSocketReceiveResult.ThrowMessageTypeOutOfRange

* Revert "Remove unused using directives"

This reverts commit 820dc04.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2024
@karelz karelz added this to the 9.0.0 milestone Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@CarnaViire CarnaViire CarnaViire approved these changes

+1 more reviewer

@zlatanov zlatanov zlatanov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@PaulusParssinen @CarnaViire @zlatanov @stephentoub @karelz