fixes for ManagedWebSocket server mode#10510
Conversation
| int maskOffset = WriteHeader(opcode, _sendBuffer, payloadBuffer, endOfMessage, useMask: true); | ||
| headerLength = maskOffset + MaskLength; | ||
| maskOffset = WriteHeader(opcode, _sendBuffer, payloadBuffer, endOfMessage, useMask: true); | ||
| headerLength = maskOffset.Value + MaskLength; |
There was a problem hiding this comment.
Slightly cheaper as GetValueOrDefault; we know it has a value s we just assigned one.
There was a problem hiding this comment.
It seems mildly odd that is the case, but looking at the code for Nullable<T> I can see why :). Will do!
|
One nit, otherwise LGTM |
We can add some unit tests around this with a minimal test-only server that pulls this file into the tests. I can look at that when I'm actually back online (pretend I'm not here :-) |
|
That's basically what I was thinking but I wasn't familiar enough with the repo structure to get much progress on that. Thanks for hopping online quick for this @stephentoub ! |
|
@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test please |
|
Looks like this is passing tests. I'm not sure exactly what the merge procedure is though. Can someone help merge this in? |
|
LGTM. We can add the tests that @stephentoub mentioned if/when we start using this in public corefx API. |
* import WebSockets code from CoreFX * sync pr feedback from dotnet/corefx#10510
5 participants
Updates to ManagedWebSocket to fix issues in server mode. There isn't really a place to put tests in corefx since no public API exercises this code path (we copy this class as raw source code into ASP.NET right now, as a stop-gap to a proper public API existing). I do have got tests for these changes in my copy of the WebSocket code, currently in PR at aspnet/WebSockets#105.
As far as breaking corefx, the only real concern is if the existing tests pass, since the new code path is not ever used by a public corefx API (I just want to keep our copy and the copy in corefx in sync).
/cc @Petermarcu (since @stephentoub is OOF, can you suggest someone to take a look at this?)
/cc @Tratcher
Fixes #10490