Call HttpContext.Abort() when WebSocket.Abort() is called. by mitchdenny · Pull Request #48892 · dotnet/aspnetcore

mitchdenny · 2023-06-19T02:30:36Z

mgravell

looks good, added a couple of questions, but on the presumption that the answer to those questions is "we're fine": 👍

mitchdenny · 2023-06-20T05:47:12Z

/backport to release/7.0

ghost · 2023-06-20T05:47:16Z

Hi @mitchdenny. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

mitchdenny · 2023-06-20T05:47:20Z

/backport to release/6.0

ghost · 2023-06-20T05:47:24Z

Hi @mitchdenny. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

github-actions · 2023-06-20T05:47:25Z

Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/5318983315

github-actions · 2023-06-20T05:47:31Z

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/5318984065

github-actions · 2023-06-20T05:47:57Z

@mitchdenny backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Call HttpContext.Abort() when WebSocket.Abort() is called.
Using index info to reconstruct a base tree...
M	src/Middleware/WebSockets/src/WebSocketMiddleware.cs
M	src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
CONFLICT (content): Merge conflict in src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Auto-merging src/Middleware/WebSockets/src/WebSocketMiddleware.cs
CONFLICT (content): Merge conflict in src/Middleware/WebSockets/src/WebSocketMiddleware.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Call HttpContext.Abort() when WebSocket.Abort() is called.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-06-20T05:47:58Z

@mitchdenny an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

github-actions · 2023-06-20T05:48:06Z

@mitchdenny backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Call HttpContext.Abort() when WebSocket.Abort() is called.
Using index info to reconstruct a base tree...
M	src/Middleware/WebSockets/src/WebSocketMiddleware.cs
M	src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
CONFLICT (content): Merge conflict in src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Auto-merging src/Middleware/WebSockets/src/WebSocketMiddleware.cs
CONFLICT (content): Merge conflict in src/Middleware/WebSockets/src/WebSocketMiddleware.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Call HttpContext.Abort() when WebSocket.Abort() is called.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-06-20T05:48:07Z

@mitchdenny an error occurred while backporting to release/6.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

amcasey · 2023-06-20T19:55:18Z

+
+    public override string? SubProtocol => _wrappedSocket.SubProtocol;
+
+    public override void Abort()


I know we use this pattern everywhere (override and add code), but what happens if someone add a virtual overload to the base type? How would we find out so we could hook that one too?

We've sometimes written tests that reflect on the type and check for anything missing. Probably not worth the effort, I don't think anything virtual has been added to this type since it was first written for .NET 4.5.

I know we use this pattern everywhere (override and add code), but what happens if someone add a virtual overload to the base type? How would we find out so we could hook that one too?

This was a big issue for streams and we built an analyzer to help detect this. WebSocket isn't pervasive enough to warrant it but cc @stephentoub.

Call HttpContext.Abort() when WebSocket.Abort() is called.

4e8819a

mitchdenny requested review from BrennanConroy and Tratcher as code owners June 19, 2023 02:30

ghost added the area-runtime label Jun 19, 2023

davidfowl reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/src/ServerWebSocket.cs

Extra memory-based overloads.

4a8e08d

mitchdenny requested a review from davidfowl June 19, 2023 06:14

martincostello reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/src/ServerWebSocket.cs Outdated

Seal ServerWebSocket.

e9795c5

mgravell reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/src/ServerWebSocket.cs

mgravell reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/src/ServerWebSocket.cs

mgravell approved these changes Jun 19, 2023

View reviewed changes

Tratcher reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs Outdated

Tratcher reviewed Jun 19, 2023

View reviewed changes

Comment thread src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs Outdated

Tratcher approved these changes Jun 19, 2023

View reviewed changes

PR feedback.

ae07ed5

mitchdenny merged commit 5ca9116 into dotnet:main Jun 20, 2023

ghost added this to the 8.0-preview6 milestone Jun 20, 2023

This was referenced Jun 20, 2023

[release/7.0] Backport pr 48892 to 7.0 #48911

Merged

[release/6.0] Backport 48892 to 6.0 #48914

Merged

amcasey reviewed Jun 20, 2023

View reviewed changes

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

BrennanConroy mentioned this pull request Aug 12, 2024

Add KeepAliveTimeout support to WebSocketMiddleware #57293

Merged


		public override string? SubProtocol => _wrappedSocket.SubProtocol;

		public override void Abort()

Conversation

mitchdenny commented Jun 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgravell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mitchdenny commented Jun 20, 2023

Uh oh!

ghost commented Jun 20, 2023

Uh oh!

mitchdenny commented Jun 20, 2023

Uh oh!

ghost commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

github-actions Bot commented Jun 20, 2023

Uh oh!

amcasey Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Tratcher Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

davidfowl Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants