search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.24k stars 4.73k forks source link

ManagedWebSocket.Abort() doesn't interrupt currently pending ReceiveAsync #44272

Open alexsku opened 3 years ago

alexsku commented 3 years ago

Description

WebSocket.Abort method is documented that it

Aborts the WebSocket connection and cancels any pending IO operations.

However it appears that when i have a pending ReceiveAsync call on that websocket and then call Abort it doesn't cancel the pending ReceiveAsync call. The call would stay pending until the other side of the connection closes the connection on their end. Instead of this I would expect ReceiveAsync to throw after Abort is called.

Here is the sample code reproducing the issue.

I was using wscat -c wss://localhost:5001/websocket --no-check to simulate a client.

Configuration

I reproduce the issue running netcoreapp3.1 in Kestrel on a windows machine. I also observed the issue running on Linux.

Regression?

I think the issue is related to https://github.com/dotnet/runtime/issues/27735. Ultimately that issue was occurring because Abort that was called when the passed CancellationToken is cancelled didn't interrupt pending ReceiveAsync call. And while the fix fixed the particular problem with the CancellationToken it didn't fix the root problem that Abort doesn't interrupt pending ReceiveAsync calls.

Other information

I think it is a pretty serious issue for a server platform. The server code should be tolerant to misbehaving clients and it should be able to interrupt connection in case if client doesn't follow the protocol. Calling Abort would be a reasonable thing to do in such cases but with this issue Abort would not interrupt the connection thus preventing the resources from being freed.

Dotnet-GitSync-Bot commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

stephentoub commented 3 years ago

ManagedWebSocket.Abort works by disposing the Stream it wraps, and it expects that will interrupt any pending operations. @halter73, we've talked about this case with Kestrel in the past; was the stream Kestrel provides ever fixed to do that?

ghost commented 3 years ago

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

stephentoub commented 3 years ago

(I expect this issue should be moved to dotnet/aspnetcore, but we can discuss first.)

halter73 commented 3 years ago

@halter73, we've talked about this case with Kestrel in the past; was the stream Kestrel provides ever fixed to do that?

No it wasn't. We recommend calling HttpContext.Abort() instead for now.

We've experimented with aborting the request when the upgrade Stream is disposed, but this causes unwanted truncation of the output during what should have been graceful closes. We can look at doing a "soft abort" instead which would cancel pending reads and/or writes facing backpressure without dropping bytes already in Kestrel's output buffers. It wouldn't be super straightforward, but it should be possible.

@Tratcher has also suggested the possibility of adding an API to ManagedWebSocket.CreateFromConnectedStream that accepts an abort-specific callback. This has the advantage of allowing us to immediately abort the underlying connection like HttpContext.Abort() does when ManagedWebSocket.Abort() is called.

karelz commented 3 years ago

Triage: Looks like this will require a new API overload on CreateFromStream -- @halter73 @Tratcher is it something you need to make this work on ASP.NET?

Tratcher commented 3 years ago

Yes, a new API is the best proposal so far. Stream.Dispose is too ambiguous between success and failure scenarios.

ghost commented 3 years ago

This issue has been automatically marked as no recent activity because it has been marked as needing more info but has not had any activity for 14 days. It will be closed if no further activity occurs within 7 more days.

stephentoub commented 3 years ago

@danmosemsft, @msftbot, do not close this.

stephentoub commented 3 years ago

@danmosemsft, are we going to have to comment on every issue we want to keep open every 14 days? How do we tell the bot "go away"?

halter73 commented 3 years ago

In the aspnetcore repo, we only do this if the issue is labeled with "Needs: Author Feedback". I wonder if Fabric Bot treating the "needs more info" label the same way in the runtime repo. I don't currently have permissions to view the Fabric Bot rules for the runtime repo, so I cannot check myself.