Aspnetcore.docs: Add guidance for WebSocket lifetime and the middleware pipeline

Created on 14 Feb 2018  路  9Comments  路  Source: dotnet/AspNetCore.Docs

See https://github.com/aspnet/Home/issues/2462

We see customers having a frequent issue with WebSockets, where they do something like this:

app.Use(async (context, next) => {
    var socket = await context.WebSockets.AcceptWebSocketAsync();
    BackgroundSocketProcessor.AddSocket(socket); 
});

BackgroundSocketProcessor is some service or component in their app that adds the socket to a collection. A background thread, or other process (other requests, etc) are then accessing those sockets and sending data. This is a common pattern and pretty much how SignalR works, so it's not entirely wrong :). However, there's a problem with this as written. After adding the socket to the background processor queue, the middleware function returns. This completes the request and causes Kestrel to conclude the Response and close the stream. As a result, when the background processor tries to send/receive on the socket, it throws an ObjectDisposedException. The message is obscure (though it will be clarified soon: https://github.com/aspnet/KestrelHttpServer/issues/2299 :tada: and https://github.com/aspnet/Home/issues/2854) and difficult to understand. We should include documentation in our WebSockets topic about how to identify this problem and deal with it.

Suggested outline.

  • New Subtopic on https://github.com/aspnet/Docs/blob/master/aspnetcore/fundamentals/websockets.md titled "WebSocket Lifetime" (does not need to be a new page, just a subheading should be sufficient)

    • The WebSocket received from AcceptWebSocketAsync lives only as long as the Middleware pipeline for that request is still running. Once the middleware pipeline ends, the socket becomes broken.

    • Suggest providing some sample exception messages, to help searchability when the exception occurs :). Easy to repro by just having a background thread try to send to one of these sockets after the middleware pipeline ends (a timer would do it).

    • If you are going to use the WebSocket from outside the request context (in a background thread or from another request) you need to ensure that the original WebSocket request does not complete until you are finished with the socket. A sample pattern for this is given below (I suggest expanding it to something more functional in the actual doc, I'm just putting a rough sketch in here right now)

    • This applies to MVC controller actions too. If you accept a socket in the action, you need to make sure you wait for the socket to complete before returning from the Action method

    • As always, never use Task.Wait() or similar blocking calls to wait for the socket to complete, it will cause serious threading issues in the application, always use await.

app.Use(async (context, next) => {
    var socket = await context.WebSockets.AcceptWebSocketAsync();
    var socketFinishedTcs = new TaskCompletionSource<object>();

    // The background processor should call socketFinishedTcs.TrySetResult(null) when it has finished with the socket
    BackgroundSocketProcessor.AddSocket(socket, socketFinishedTcs); 

    // Wait for the BackgroundSocketProcessor to finish with the socket before concluding the request.
    await socketFinishedTcs.Task;
});
P1 doc-enhancement

Most helpful comment

Can you add these exceptions to this issue

I don't have the latest message on-hand unfortunately. It's pretty easy to force this problem to occur by doing something like this:

app.Use(async (context, next) => {
    var socket = await context.WebSockets.AcceptWebSocketAsync();
    Task.Run(async () => {
        await Task.Delay(1000);
        try {
            await socket.SendAsync(...); // This should throw the exception
        } catch(Exception ex) {
            // Put a breakpoint here and you should see the exception
        }
    });
});

The code starts a background task, ends the request and then 1 second later (after the request has ended) the background task writes to the socket and should throw the exception. I believe it should be an ObjectDisposedException. The message may be a little clearer now (I think we changed it to something like "Attempted to write to the response after the request ended")

All 9 comments

@anurse

Suggest providing some sample exception messages, to help searchability when the exception occurs

Can you add these exceptions to this issue and I'll try to squeeze this issue into this sprint.

@tdykstra review and assign to PU or accept.

@Rick-Anderson Looks like there's enough to go on here to add the requested section to the doc. Sample error messages would be helpful, but if we don't have them we can go with what we have.

Can you add these exceptions to this issue

I don't have the latest message on-hand unfortunately. It's pretty easy to force this problem to occur by doing something like this:

app.Use(async (context, next) => {
    var socket = await context.WebSockets.AcceptWebSocketAsync();
    Task.Run(async () => {
        await Task.Delay(1000);
        try {
            await socket.SendAsync(...); // This should throw the exception
        } catch(Exception ex) {
            // Put a breakpoint here and you should see the exception
        }
    });
});

The code starts a background task, ends the request and then 1 second later (after the request has ended) the background task writes to the socket and should throw the exception. I believe it should be an ObjectDisposedException. The message may be a little clearer now (I think we changed it to something like "Attempted to write to the response after the request ended")

I'm seeing a WebSocketException:

The remote party closed the WebSocket connection without completing the close handshake.

with ObjectDisposedException inner exception:

Cannot write to the response body, the response has completed.
Object name: 'HttpResponseStream'.

That's the one

A sample pattern for this is given below (I suggest expanding it to something more functional in the actual doc, I'm just putting a rough sketch in here right now)

@anurse Is the "rough sketch" code good enough to use now and plug in the "more functional" code later when it's available?

I think the rough sketch is actually fine as-is. No need to refine it further.

Looks like this issue was mostly taken care of by #12378
I can expand on that by adding the code sample from this issue.

Was this page helpful?
0 / 5 - 0 ratings