Call HttpContext.Abort() when WebSocket.Abort() is called. #48892
Conversation
ghost
added
the
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
looks good, added a couple of questions, but on the presumption that the answer to those questions is "we're fine": 👍
src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
src/Middleware/WebSockets/test/UnitTests/WebSocketMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
|
/backport to release/7.0 |
|
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. |
|
/backport to release/6.0 |
|
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. |
|
Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/5318983315 |
|
Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/5318984065 |
|
@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 128Please backport manually! |
|
@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. |
|
@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 128Please backport manually! |
|
@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. |
|
|
||
| public override string? SubProtocol => _wrappedSocket.SubProtocol; | ||
|
|
||
| public override void Abort() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
amcasey
added
area-networking
6 participants
Related #39519