hub: rewrite code for dealing with mutiple websocket upgrade handlers at once, since this broke with the nextjs upgrade

Author: williamstein

src/packages/hub/hub.ts

@@ -236,11 +236,6 @@ async function startServer(): Promise<void> {
       nextServer: !!program.nextServer,
       cert: program.httpsCert,
       key: program.httpsKey,
-      listenersHack:
-        program.mode == "single-user" &&
-        program.proxyServer &&
-        program.nextServer &&
-        process.env["NODE_ENV"] == "development",
     });
 
     // The express app create via initExpressApp above **assumes** that init_passport is done

src/packages/hub/proxy/handle-upgrade.ts

@@ -11,10 +11,12 @@ import { versionCheckFails } from "./version";
 import { proxyConatWebsocket } from "./proxy-conat";
 import basePath from "@cocalc/backend/base-path";
 
+const LISTENERS_HACK = true;
+
 const logger = getLogger("proxy:handle-upgrade");
 
-export default function init(
-  { projectControl, isPersonal, httpServer, listenersHack, proxyConat },
+export default function initUpgrade(
+  { projectControl, isPersonal, httpServer, proxyConat },
   proxy_regexp: string,
 ) {
   const cache = new LRU<string, ProxyServer>({
@@ -24,22 +26,72 @@ export default function init(
 
   const re = new RegExp(proxy_regexp);
 
+  let nextUpgrade: undefined | Function = undefined;
+  let socketioUpgrade: undefined | Function = undefined;
+
   async function handleProxyUpgradeRequest(req, socket, head): Promise<void> {
-    if (proxyConat) {
-      const u = new URL(req.url, "http://cocalc.com");
-      let pathname = u.pathname;
-      if (basePath.length > 1) {
-        pathname = pathname.slice(basePath.length);
-      }
-      if (pathname == "/conat/") {
-        proxyConatWebsocket(req, socket, head);
-        return;
+    if (LISTENERS_HACK) {
+      const v = getEventListeners(httpServer, "upgrade");
+      if (v.length > 1) {
+        // Nodejs basically assumes that there is only one listener for the "upgrade" handler,
+        // but depending on how you run CoCalc, two others may get added:
+        //    - a socketio server
+        //    - a nextjs server
+        // We check if anything extra got added and if so, identify it and properly
+        // use it.  We identify the handle using `${f}` and using a heuristic for the
+        // code. That's the best I can do and it's obviously brittle.
+        // Note: rspack for the static app doesn't use a websocket, instead using SSE, so
+        // fortunately it's not relevant and hmr works fine.  HMR for the nextjs server
+        // tends to just refresh the page, probably because we're using rspack there too.
+        for (const f of v) {
+          if (f === handler) {
+            // it's us -- leave it alone
+            continue;
+          }
+          const source = `${f}`;
+          logger.debug(`found extra listener`, { f, source });
+          if (source.includes("destroyUpgrade")) {
+            // WARNING/BRITTLE! the socketio source code for the upgrade handler has a destroyUpgrade
+            // option it checks for, whereas the nextjs one doesn't.
+            if (socketioUpgrade === undefined) {
+              socketioUpgrade = f;
+            } else {
+              logger.debug(
+                "WARNING! discovered unknown upgrade listener!",
+                source,
+              );
+            }
+          } else {
+            if (nextUpgrade === undefined) {
+              nextUpgrade = f;
+            } else {
+              logger.debug(
+                "WARNING! discovered unknown upgrade listener!",
+                source,
+              );
+            }
+          }
+          logger.debug(
+            `found extra listener -- detected, saved and removed 'upgrade' listener`,
+            source,
+          );
+          httpServer.removeListener("upgrade", f);
+        }
       }
     }
 
+    if (proxyConat && useSocketio(req.url)) {
+      proxyConatWebsocket(req, socket, head);
+      return;
+    }
+
     if (!req.url.match(re)) {
-      // something else (e.g., the socket.io server) is handling this websocket;
-      // we do NOT mess with anything in this case
+      // it's to be handled by socketio or next
+      if (socketioUpgrade !== undefined && useSocketio(req.url)) {
+        socketioUpgrade(req, socket, head);
+        return;
+      }
+      nextUpgrade?.(req, socket, head);
       return;
     }
 
@@ -128,80 +180,15 @@ export default function init(
     proxy.ws(req, socket, head);
   }
 
-  let handler;
-  if (listenersHack) {
-    // This is an insane horrible hack to fix https://github.com/sagemathinc/cocalc/issues/7067
-    // The problem is that there are four separate websocket "upgrade" handlers when we are doing
-    // development, and nodejs just doesn't have a good solution to multiple websocket handlers,
-    // as explained here: https://github.com/nodejs/node/issues/6339
-    // The four upgrade handlers are:
-    //   - this proxy here
-    //   - the main hub primus one
-    //   - the HMR reloader for that static webpack server for the app
-    //   - the HMR reloader for nextjs
-    // These all just sort of randomly fight for any incoming "upgrade" event,
-    // and if they don't like it, tend to try to kill the socket.  It's totally insane.
-    // What's worse is that getEventListeners only seems to ever return *two*
-    // listeners.  By extensive trial and error, it seems to return first the primus
-    // listener, then the nextjs one.  I have no idea why the order is that way; I would
-    // expect the reverse.  (Update: it's because nextjs uses a hack -- it only installs
-    // a listener once a request comes in. Until there is a request, nextjs does not have
-    // access to the server and can't mess with it.)
-    // And I don't know why this handler here isn't in the list.
-    // In any case, once we get a failed request *and* we see there are at least two
-    // other handlers (it's exactly two), we completely steal handling of the upgrade
-    // event here.  We then call the appropriate other handler when needed.
-    // I have no idea how the HMR reloader for that static webpack plays into this,
-    // but it appears to just work for some reason.
-
-    // NOTE: I had to do something similar that is in packages/next/lib/init.js,
-    // and is NOT a hack.  That technique could probably be used to fix this properly.
-    // NOTE2: It's May 2025, and I basically don't use HMR anymore and just refresh
-    // my page, since dealing with this is so painful.  Also rspack is superfast and
-    // refresh is fast, so HMR feels less necessary.  Finally, frequently any dev work
-    // I do requires a page refresh anyways.
-
-    let listeners: any[] = [];
-    handler = async (req, socket, head) => {
-      logger.debug("Proxy websocket handling -- using listenersHack");
-      try {
-        await handleProxyUpgradeRequest(req, socket, head);
-      } catch (err) {
-        if (listeners.length == 0) {
-          const x = getEventListeners(httpServer, "upgrade");
-          if (x.length >= 2) {
-            logger.debug(
-              "Proxy websocket handling -- installing listenersHack",
-            );
-            listeners = [...x];
-            httpServer.removeAllListeners("upgrade");
-            httpServer.on("upgrade", handler);
-          }
-        }
-        if (req.url.includes("hub?_primus") && listeners.length >= 2) {
-          listeners[0](req, socket, head);
-          return;
-        }
-        if (req.url.includes("_next/webpack-hmr") && listeners.length >= 2) {
-          listeners[1](req, socket, head);
-          return;
-        }
-        const msg = `WARNING: error upgrading websocket url=${req.url} -- ${err}`;
-        logger.debug(msg);
-        denyUpgrade(socket);
-      }
-    };
-  } else {
-    handler = async (req, socket, head) => {
-      try {
-        await handleProxyUpgradeRequest(req, socket, head);
-      } catch (err) {
-        const msg = `WARNING: error upgrading websocket url=${req.url} -- ${err}`;
-        logger.debug(msg);
-        denyUpgrade(socket);
-      }
-    };
-  }
+  const handler = async (req, socket, head) => {
+    try {
+      await handleProxyUpgradeRequest(req, socket, head);
+    } catch (err) {
+      const msg = `WARNING: error upgrading websocket url=${req.url} -- ${err}`;
+      logger.debug(msg);
+      denyUpgrade(socket);
+    }
+  };
 
   return handler;
 }
@@ -210,3 +197,12 @@ function denyUpgrade(socket) {
   socket.write("HTTP/1.1 401 Unauthorized\r\n\r\n");
   socket.destroy();
 }
+
+function useSocketio(url: string) {
+  const u = new URL(url, "http://cocalc.com");
+  let pathname = u.pathname;
+  if (basePath.length > 1) {
+    pathname = pathname.slice(basePath.length);
+  }
+  return pathname == "/conat/";
+}

src/packages/hub/proxy/index.ts

@@ -5,7 +5,7 @@
 
 import { Application } from "express";
 import getLogger from "../logger";
-import initProxy from "./handle-request";
+import initRequest from "./handle-request";
 import initUpgrade from "./handle-upgrade";
 import base_path from "@cocalc/backend/base-path";
 import { ProjectControlFunction } from "@cocalc/server/projects/control";
@@ -17,18 +17,17 @@ interface Options {
   httpServer; // got from express_app via httpServer = http.createServer(app).
   projectControl: ProjectControlFunction; // controls projects (aka "compute server")
   isPersonal: boolean; // if true, disables all access controls
-  listenersHack: boolean;
   proxyConat: boolean;
 }
 
-export default function init(opts: Options) {
+export default function initProxy(opts: Options) {
   const proxy_regexp = `^${
     base_path.length <= 1 ? "" : base_path
   }\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}\/*`;
   logger.info("creating proxy server with proxy_regexp", proxy_regexp);
 
   // tcp connections:
-  const handleProxy = initProxy(opts);
+  const handleProxy = initRequest(opts);
 
   // websocket upgrades:
   const handleUpgrade = initUpgrade(opts, proxy_regexp);

src/packages/hub/servers/app/next.ts

@@ -11,7 +11,6 @@ import { join } from "path";
 
 // @ts-ignore -- TODO: typescript doesn't like @cocalc/next/init (it is a js file).
 import initNextServer from "@cocalc/next/init";
-
 import basePath from "@cocalc/backend/base-path";
 import { getLogger } from "@cocalc/hub/logger";
 import handleRaw from "@cocalc/next/lib/share/handle-raw";

src/packages/hub/servers/express-app.ts

@@ -45,7 +45,6 @@ interface Options {
   conatServer: boolean;
   cert?: string;
   key?: string;
-  listenersHack: boolean;
 }
 
 export default async function init(opts: Options): Promise<{
@@ -150,19 +149,19 @@ export default async function init(opts: Options): Promise<{
     });
   }
 
+  // This must be second to the last, since it will prevent any
+  // other upgrade handlers from being added to httpServer.
   if (opts.proxyServer) {
     winston.info(`initializing the http proxy server`, {
       conatSocketioCount,
       conatServer: !!opts.conatServer,
       isPersonal: opts.isPersonal,
-      listenersHack: opts.listenersHack,
     });
     initProxy({
       projectControl: opts.projectControl,
       isPersonal: opts.isPersonal,
       httpServer,
       app,
-      listenersHack: opts.listenersHack,
       // enable proxy server for /conat if:
       //  (1) we are not running conat at all from here, or
       //  (2) we are running socketio in cluster mode, hence

src/packages/next/next.config.js

@@ -31,6 +31,9 @@ const config = {
       "node_modules",
       "react-dom",
     );
+    config.devServer = {
+      hot: true,
+    };
     // Important: return the modified config
     return config;
   },

src/packages/next/pages/index.tsx

@@ -6,7 +6,6 @@
 import { Layout, Tooltip } from "antd";
 import { GetServerSidePropsContext } from "next";
 import { join } from "path";
-
 import { getRecentHeadlines } from "@cocalc/database/postgres/news";
 import { COLORS } from "@cocalc/util/theme";
 import { RecentHeadline } from "@cocalc/util/types/news";
@@ -84,7 +83,7 @@ export default function Home(props: Props) {
         }}
       >
         <Title level={1} style={{ color: COLORS.GRAY }}>
-          Signed in as{" "}
+        Signed in as{" "}
           <Tooltip title={"View all your account settings"} placement={"right"}>
             <a href={join(basePath, "settings")}>
               {`${account.first_name} ${account.last_name} ${