WebSocket instability

[jflatow]

Version: at least Deno 1.39.1 through 1.40.3

This is related to #17283 and probably other issues, but recently WebSockets seem to have gotten more unstable. Simply refreshing pages or navigating in a browser which is connected via a WebSocket to a Deno server can lead to various crashes of the form: Upgrade response was not returned from callback. This seems like a regression as it was previously relatively stable (except when using TLS).

[e3dio]

This error Upgrade response was not returned from callback https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L483 happens for a specific case: using Deno.serve and Deno.upgradeWebSocket and then not returning the same response that was returned by Deno.upgradeWebSocket. Does your upgrade route return the correct response object in all cases ?

const handler = req => {
   const { socket, response } = Deno.upgradeWebSocket(req);
   return response;
}

I don't know about it crashing the server though you would need to provide more info, it should just close the client connection if correct response object is not returned

[bartlomieju]

CC @mmastrac @littledivy

[littledivy]

We would need some code (atleast the serve handler) to investigate this.

The error occurs when the response returned from the serve handler is not the same one returned by Deno.upgradeWebSocket. This is intended behaviour.

[jflatow]

This error Upgrade response was not returned from callback https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L483 happens for a specific case: using Deno.serve and Deno.upgradeWebSocket and then not returning the same response that was returned by Deno.upgradeWebSocket. Does your upgrade route return the correct response object in all cases ?

const handler = req => {
   const { socket, response } = Deno.upgradeWebSocket(req);
   return response;
}

I don't know about it crashing the server though you would need to provide more info, it should just close the client connection if correct response object is not returned

I have no branches and always return this response, and the code I have hasn't changed and was previously stable, so I'm not sure (and it does fully crash now). I will work on producing a minimal example, but I don't think you should just dismiss this.

[e3dio]

I have no branches and always return this response

I do see an issue/bug with the new Deno.serve() and websocket route: if the route throws an error before response gets returned the server silently stops accepting requests and process exits if nothing else is running. I'm guessing your route threw an error at some point before returning response

const port = 3003;

const server = Deno.serve({ port }, req => {
   const { socket, response } = Deno.upgradeWebSocket(req);
   throw 'throw error test';
   return response;
});

const ws = new WebSocket(`ws://127.0.0.1:${port}`);

server.finished.then(() => console.log('server exited'));

throw error test Upgrade response was not returned from callback server exited

https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L468-L487

[mmastrac]

@e3dio Great catch. I think if a connection has been upgraded and the handler throws we should avoid calling the onError handler and treat it as success.

[e3dio]

It's tricky because the connection has been 'upgraded' but the websocket doesn't finish opening server-side until the response is successfully returned https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L206 https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L486 If an error is thrown before response is returned, it's likely the user would prefer the connection to fail and close rather than continue opening websocket. The server needs to return a generated 500 response and not stop serving requests

[e3dio]

I'm wondering if it would be simpler to wait until response is returned from callback before calling op_http_upgrade_websocket_next That way the upgrade happens in 1 step at same time, instead of websocket existing in half-open state. Currently websocket is open according to client, but not officially open on server until response is returned. There could be a delay that causes issues

[e3dio]

I see this goAhead gets resolved before the response is returned so it does fully open the websocket before response returned. I don't see where this is getting resolved though, it's not getting called from the only place I see it can get called which is after the returned response: innerRequest?.#upgraded()

[mmastrac]

Good point. The goAhead signal is triggered when the response is returned. This ensures the WebSocket cannot be used until the response transaction has completed.

This is mostly for compatibility with the existing Deno.serveHttp implementation. But since the WebSocket is open-but-technically-inert until the response is returned, we can probably just abort the WebSocket if the response handler throws.

[e3dio]

But since the WebSocket is open-but-technically-inert until the response is returned

goAhead is getting called before the response is returned, I don't see where it's called from yet. So technically the websocket can be fully open and ready to use before the response object is returned in callback, I tested by doing logging and waiting before returning response and it works.

A quick fix for current issue is to close websocket connection if error in request callback, and not stop server from accepting new requests. Whether the websocket should officially be upgraded during Deno.upgradeWebSocket(req) call or after response is returned from callback is a different question. Probably makes more sense to wait until response is returned, then it can follow the same request/response/error pipeline as a standard request

[e3dio]

Ok I think I see mistake in code, this should be await goAhead.promise not await goAhead https://github.com/denoland/deno/blob/main/ext/http/00_serve.js#L207 https://github.com/denoland/deno/blob/main/ext/web/06_streams.js#L125

So it doesn't actually wait for the goAhead promise

[e3dio]

So you have mistake in code there that triggered WS open early. But even if you fix that and await goAhead.promise now you have half-open websocket, open on client side but not officially on server. Ideally connection upgrade should happen after response is returned. You able to fix all these things or you need some help with that

[mmastrac]

I'll PR a fix for the goAhead bug to start -- that's definitely not behaving as we expect.

[pagoru]

This is not a bug, this error happens when you upgradeWebSocket the request and then, return a new response. The response needs to be from the socket, because you already upgraded the request to a socket, so it makes sense to throw that error.

Edit: This is a bug

[jflatow]

This is not a bug, this error happens when you upgradeWebSocket the request and then, return a new response. The response needs to be from the socket, because you already upgraded the request to a socket, so it makes sense to throw that error.

No, see above, this is/was definitely a bug since it was happening when the upgrade response was being returned correctly.

[pagoru]

This is not a bug, this error happens when you upgradeWebSocket the request and then, return a new response. The response needs to be from the socket, because you already upgraded the request to a socket, so it makes sense to throw that error.

No, see above, this is/was definitely a bug since it was happening when the upgrade response was being returned correctly.

Ups, sorry! I had a similar problem, but was my fault because I was modifying the response.

If anyone happens to reach this and are modifying the response or sending a new response, Don't do it!