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

Add additional close statuses in ManagedWebSocket #83827

Merged
merged 9 commits into from
Mar 31, 2023

Conversation

IDisposable
Copy link
Contributor

@IDisposable IDisposable commented Mar 23, 2023

Add ServiceRestart (1012), TryAgainLater (1013), and BadGateway (1014) to the list of WebSocketCloseStatus values and allow them to be used as valid WebSocket close statuses so we don't reject the close and discard the status description by adding them to the private IsValueCloseStatus method switch statement declaring them as valid true.

Fixes #82602

Description

The current implementation of ManagedWebSocket explicitly declares some WebSocketCloseStatus values as "acceptable" reasons to close the socket. For those, the information status description is extracted and made available. For all other values, the close status is rejected and the closing information is discarded.

This means that things like BadGateway (1014), or ServiceRestart (1012), or TryAgainLater (1013) will be declared invalid by ManagedWebSocket.IsValidCloseStatus and thus not handled cleanly even though they could happen after a web socket is completely setup. These codes are documented here as valid server-initiated close reasons.

Customer Impact

Lost information and ragged closing of a web socket when a server-side or proxy closes because of any of the previously rejected values:

  • ServiceRestart = 1012 // indicates that the server / service is restarting.
  • TryAgainLater = 1013 // indicates that a temporary server condition forced blocking the client's request.
  • BadGateway = 1014 // indicates that the server acting as gateway received an invalid response

This codes are documented here and here as IANA registered and in the Mozilla docs

Regression

No

Testing

Added test cases for all three to new WebSocketCloseTests.cs file with no regressions in current tests.

Risk

This does not change the public enum of WebSocketCloseStatus as we don't want to invoke public review and the values are not mentioned in the RFC.

Package authoring signed off?

N/A

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 23, 2023
@ghost
Copy link

ghost commented Mar 23, 2023

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

Issue Details

Add ServiceRestart (1012), TryAgainLater (1013), and BadGateway (1014) to the list of WebSocketCloseStatus values and allow them to be used as valid WebSocket close statuses so we don't reject the close and discard the status description by adding them to the private IsValueCloseStatus method switch statement declaring them as valid true.

Fixes Issue #82602

Description

The current implementation of ManagedWebSocket explicitly declares some WebSocketCloseStatus values as "acceptable" reasons to close the socket. For those, the information status description is extracted and made available. For all other values, the close status is rejected and the closing information is discarded.

This means that things like BadGateway (1014), or ServiceRestart (1012), or TryAgainLater (1013) will be declared invalid by ManagedWebSocket.IsValidCloseStatus and thus not handled cleanly even though they could happen after a web socket is completely setup. These codes are documented here as valid server-initiated close reasons.

Customer Impact

Lost information and ragged closing of a web socket when a server-side or proxy closes because of any of the previously rejected values:

  • ServiceRestart = 1012 // indicates that the server / service is restarting.
  • TryAgainLater = 1013 // indicates that a temporary server condition forced blocking the client's request.
  • BadGateway = 1014 // indicates that the server acting as gateway received an invalid response

This codes are documented here and here as IANA registered and in the Mozilla docs

Regression

No

Testing

Added test cases for all three to new WebSocketCloseTests.cs file with no regressions in current tests.

Risk

This does not change the public enum of WebSocketCloseStatus as we don't want to invoke public review and the values are not mentioned in the RFC.

Package authoring signed off?

N/A

Author: IDisposable
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

@IDisposable
Copy link
Contributor Author

IDisposable commented Mar 23, 2023

This replaces #83713

@IDisposable IDisposable marked this pull request as ready for review March 23, 2023 16:49
@IDisposable
Copy link
Contributor Author

@wfurt and @CarnaViire I have redone this without the public enum change. Let me know if there's a better test (I don't know the WebSockets protocol at all, so I didn't build up native packets, sorry).

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. thanks @IDisposable

We could make the extra codes private static const inside of ManagedWebSocket.cs but I don't have strong feelings about it. The extra comment seems sufficient to me.

@IDisposable
Copy link
Contributor Author

IDisposable commented Mar 24, 2023

That's an interesting idea @wfurt. Could/would it be an internal enum or bundle of static const int values in an adjacent .cs file be more appropriate since we need the constants in both ManagedWebSocket and in the test? I've not been into the runtime code until very recently so not sure what the running practices are... I could craft it up both ways on a couple branches for comment if desired.

Looks like doing the internal would make the test unable to see the enum, so this is totally not worth-while.

@wfurt
Copy link
Member

wfurt commented Mar 24, 2023

I would wait for feedback from others...

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 27, 2023
@IDisposable IDisposable force-pushed the allow-nonstandard-closes branch 2 times, most recently from 9199e46 to d0d600e Compare March 27, 2023 21:41
@IDisposable
Copy link
Contributor Author

IDisposable commented Mar 27, 2023

Thanks for all the feedback @MartyIX, @wfurt, and @CarnaViire.
I think this ready for another round of feedback or approval.
Thanks for teaching me how to fish!

(not sure if I need to do anything with that needs-author-action, which seems to be caused by me accepting @CarnaViire's suggestions)

@CarnaViire
Copy link
Member

Could you please update your branch to the current main @IDisposable ? I saw build failures that seem to be caused by branch being out of date

IDisposable and others added 4 commits March 30, 2023 14:45
Add ServiceRestart (1012), TryAgainLater (1013), and BadGateway (1014) to the list of `WebSocketCloseStatus` values and allow them to be used as valid WebSocket close statuses so we don't reject the close and discard the status description by adding them to the private `IsValueCloseStatus` method switch statement declaring them as valid `true`. 
These codes are documented [here as IANA registered codes](https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent/code) as valid server-initiated close reasons.

Fixes Issue dotnet#82602
Co-authored-by: Natalia Kondratyeva <[email protected]>
IDisposable and others added 5 commits March 30, 2023 14:53
Finished rename of CloseStatuses test data
Renamed `closeStatusDescription` to `serverMessage`
Send hello message and close status then await both responses and
check they are as expected. This necessitated switching to the `ReceiveAsync` that accepts an `ArraySegment`.
Explicitly typed `var`s
Inlined helper methods (for clarity)
Swapping back to a distinct and more appropriately named variable for the `closeStatusDescription`

Co-authored-by: Natalia Kondratyeva <[email protected]>
Co-authored-by: Natalia Kondratyeva <[email protected]>
Renamed local `serverMessage` back to `closeStatusDescription` per PR feedback.
Co-authored-by: Natalia Kondratyeva <[email protected]>
Rebased to current main, updated the commit messages and added the remaining changes to address the PR comments.
@IDisposable
Copy link
Contributor Author

I've addressed all the PR comments and rebased to current main, but it won't build on my machine anymore... doing a Full build now to see if things will work, but figured I could push up and let the CI try,

I get this when building the .SLN in the directory (worked before today):

Error	MSB4018	The "Microsoft.DotNet.ApiCompat.Task.ValidateAssembliesTask" task failed unexpectedly.
System.MissingMethodException: Method not found: 'Void Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions..ctor(Microsoft.CodeAnalysis.OutputKind, Boolean, System.String, System.String, System.String, System.Collections.Generic.IEnumerable`1<System.String>, Microsoft.CodeAnalysis.OptimizationLevel, Boolean, Boolean, System.String, System.String, System.Collections.Immutable.ImmutableArray`1<Byte>, System.Nullable`1<Boolean>, Microsoft.CodeAnalysis.Platform, Microsoft.CodeAnalysis.ReportDiagnostic, Int32, System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<System.String,Microsoft.CodeAnalysis.ReportDiagnostic>>, Boolean, Boolean, Microsoft.CodeAnalysis.XmlReferenceResolver, Microsoft.CodeAnalysis.SourceReferenceResolver, Microsoft.CodeAnalysis.MetadataReferenceResolver, Microsoft.CodeAnalysis.AssemblyIdentityComparer, Microsoft.CodeAnalysis.StrongNameProvider, Boolean, Microsoft.CodeAnalysis.MetadataImportOptions, Microsoft.CodeAnalysis.NullableContextOptions)'.
   at Microsoft.DotNet.ApiSymbolExtensions.AssemblySymbolLoader..ctor(Boolean resolveAssemblyReferences)
   at Microsoft.DotNet.ApiSymbolExtensions.AssemblySymbolLoaderFactory.Create(Boolean shouldResolveReferences) in /_/src/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoaderFactory.cs:line 12
   at Microsoft.DotNet.ApiCompatibility.Runner.ApiCompatRunner.CreateAssemblySymbols(IReadOnlyList`1 metadataInformation, ApiCompatRunnerOptions options, Boolean& resolvedExternallyProvidedAssemblyReferences) in /_/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Runner/ApiCompatRunner.cs:line 123
   at Microsoft.DotNet.ApiCompatibility.Runner.ApiCompatRunner.ExecuteWorkItems() in /_/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Runner/ApiCompatRunner.cs:line 49
   at Microsoft.DotNet.ApiCompat.ValidateAssemblies.Run(Func`2 logFactory, Boolean generateSuppressionFile, String[] suppressionFiles, String suppressionOutputFile, String noWarn, Boolean respectInternals, Boolean enableRuleAttributesMustMatch, String[] excludeAttributesFiles, Boolean enableRuleCannotChangeParameterName, String[] leftAssemblies, String[] rightAssemblies, Boolean enableStrictMode, String[][] leftAssembliesReferences, String[][] rightAssembliesReferences, Boolean createWorkItemPerAssembly, ValueTuple`2[] leftAssembliesTransformationPatterns, ValueTuple`2[] rightAssembliesTransformationPatterns) in /_/src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/ValidateAssemblies.cs:line 90
   at Microsoft.DotNet.ApiCompat.Task.ValidateAssembliesTask.ExecuteCore() in /_/src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs:line 127
   at Microsoft.NET.Build.Tasks.TaskBase.Execute() in /_/src/Tasks/Common/TaskBase.cs:line 52
   at Microsoft.DotNet.ApiCompat.Task.ValidateAssembliesTask.Execute() in /_/src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs:line 117
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()	System.Net.WebSockets (src\System.Net.WebSockets)	C:\Users\idisp\.nuget\packages\microsoft.dotnet.apicompat.task\8.0.100-preview.2.23107.1\build\Microsoft.DotNet.ApiCompat.ValidateAssemblies.Common.targets	16	

@CarnaViire
Copy link
Member

doing a Full build now to see if things will work

Yeah, when you rebase, it's better to rebuild everything from scratch (git clean -xdf and then .\build.cmd clr+libs -rc release), otherwise the old binaries might not be compatible.

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!

@CarnaViire
Copy link
Member

CarnaViire commented Mar 31, 2023

I see that CLA check is stuck (it is a known issue).
I'm going to close and then reopen the PR to see if it helps.

UPD: it helped, but the tests will be re-run.

@CarnaViire CarnaViire closed this Mar 31, 2023
@CarnaViire CarnaViire reopened this Mar 31, 2023
@CarnaViire
Copy link
Member

Test failure is unrelated #83655

@CarnaViire CarnaViire merged commit ef6b6d5 into dotnet:main Mar 31, 2023
@chenxinyanc
Copy link

Thanks for the fix! Just curious, will this change get backported to .NET 6 or do I need to switch to .NET 8 to consume the fix?

@CarnaViire
Copy link
Member

The chance of a backport is extremely low, unless there's a significant business impact. Given that currently there's only one hit, I assume you would need to switch to .NET 8 @chenxinyanc

@IDisposable
Copy link
Contributor Author

If, by chance, we do need a backport, I would love to learn the process to do that as well.

@IDisposable IDisposable deleted the allow-nonstandard-closes branch April 3, 2023 20:05
@ghost ghost locked as resolved and limited conversation to collaborators May 4, 2023
@karelz karelz added this to the 8.0.0 milestone May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
6 participants