Not setting upgrade header correctly results in client request timeout

[aral]

Hi there,

Just ran into this hairy issue that took me a while to narrow down while using tinyws in Kitten with Polka:

https://codeberg.org/kitten/app/issues/113

Basically, the WebSocket routes on my https server were timing out after 120 seconds (the default timeout for Node https servers) with a 1006 error. The upgrade was happening correctly and the socket was otherwise fine. If I listened to the clientError event on the server, I could work around the issue and prevent the socket from closing but I wanted to get to the bottom of it and fix it properly (of course).

The problem is that it doesn’t seem to happen in a very basic reproduction attempt using tinyws or even tinyws + polka.

However, through lots of trial an error, I was able to narrow down the issue to how tinyws is setting the upgrade header.

TL;DR: Setting Buffer.alloc(0) instead of actually using a copy of the header that it receives. Mostly because, since it doesn’t handle the upgrade event itself, it doesn’t have access to the actual header.

So I’m not sure if you can fix this without essentially transforming tinyws into express-websocket but I thought I’d open an issue and document it here too.

My initial implementation is a mix of tinyws and express-websocket. Please feel free to copy/adapt it if you like:

https://codeberg.org/kitten/app/src/branch/main/src/Server.js#L50

[talentlessguy]

Thanks for the bug report, I looked at your implementation and I don't mind it being adapted - it's a serious issue.

PR with the improved implementation is welcome :D

[macalinao]

Thanks for the help @aral

I went and built a function to do this on a Tiny http server here:

import * as http from "node:http";
import type { Socket } from "node:net";

import type { App, Request, Response } from "@tinyhttp/app";
import type { WebSocket } from "ws";
import { WebSocketServer } from "ws";

export interface TinyWSRequest extends http.IncomingMessage {
  ws: () => Promise<WebSocket>;
}

export const setupTinyWS = <
  Req extends Request & TinyWSRequest,
  Res extends Response,
>(
  tiny: App<Req, Res>,
  server: http.Server,
) => {
  const wss = new WebSocketServer({ noServer: true });

  const upgradeHandler = (
    request: http.IncomingMessage,
    socket: Socket,
    head: Buffer,
  ) => {
    const response = new http.ServerResponse(request);
    response.assignSocket(socket);

    // Avoid hanging onto upgradeHead as this will keep the entire
    // slab buffer used by node alive.
    const copyOfHead = Buffer.alloc(head.length);
    head.copy(copyOfHead);

    response.on("finish", function () {
      if (response.socket !== null) {
        response.socket.destroy();
      }
    });

    /**
      Mix in WebSocket to request object.

      @typedef {{ ws: function }} WebSocketMixin
      @typedef { http.IncomingMessage & WebSocketMixin } IncomingMessageWithWebSocket
    */
    (request as TinyWSRequest).ws = () =>
      new Promise((resolve) => {
        wss.handleUpgrade(request, request.socket, copyOfHead, (ws) => {
          wss.emit("connection", ws, request);
          resolve(ws);
        });
      });

    tiny.handler(request as Req, response as Res);
  };

  server.on("upgrade", upgradeHandler);
};

Not as elegant as it's no longer directly a middleware, but at least we can still use TinyHTTP.

[c0bra]

Thanks for the help @aral

I went and built a function to do this on a Tiny http server here:

import * as http from "node:http";
import type { Socket } from "node:net";

import type { App, Request, Response } from "@tinyhttp/app";
import type { WebSocket } from "ws";
import { WebSocketServer } from "ws";

export interface TinyWSRequest extends http.IncomingMessage {
  ws: () => Promise<WebSocket>;
}

export const setupTinyWS = <
  Req extends Request & TinyWSRequest,
  Res extends Response,
>(
  tiny: App<Req, Res>,
  server: http.Server,
) => {
  const wss = new WebSocketServer({ noServer: true });

  const upgradeHandler = (
    request: http.IncomingMessage,
    socket: Socket,
    head: Buffer,
  ) => {
    const response = new http.ServerResponse(request);
    response.assignSocket(socket);

    // Avoid hanging onto upgradeHead as this will keep the entire
    // slab buffer used by node alive.
    const copyOfHead = Buffer.alloc(head.length);
    head.copy(copyOfHead);

    response.on("finish", function () {
      if (response.socket !== null) {
        response.socket.destroy();
      }
    });

    /**
      Mix in WebSocket to request object.

      @typedef {{ ws: function }} WebSocketMixin
      @typedef { http.IncomingMessage & WebSocketMixin } IncomingMessageWithWebSocket
    */
    (request as TinyWSRequest).ws = () =>
      new Promise((resolve) => {
        wss.handleUpgrade(request, request.socket, copyOfHead, (ws) => {
          wss.emit("connection", ws, request);
          resolve(ws);
        });
      });

    tiny.handler(request as Req, response as Res);
  };

  server.on("upgrade", upgradeHandler);
};

Not as elegant as it's no longer directly a middleware, but at least we can still use TinyHTTP.

Thanks so much for this.

With a couple tweaks I was able to get this working with Express:

tiny: App<Req, Res> -> app: Express tiny.handler(request as Req, response as Res) -> app(request as Req, response as Res);

[talentlessguy]

I don't mind if it stops being a middleware as long as it does the job. so PR with a proper implementation is welcome (and don't forget to add tests)