hub: refactor file download handler and make hub proxy error handling more robust

Author: williamstein

src/packages/hub/proxy/file-download.ts

@@ -0,0 +1,71 @@
+import { readFile as readProjectFile } from "@cocalc/conat/files/read";
+import { once } from "events";
+import { path_split } from "@cocalc/util/misc";
+import mime from "mime-types";
+import getLogger from "../logger";
+
+const DANGEROUS_CONTENT_TYPE = new Set(["image/svg+xml" /*, "text/html"*/]);
+
+const logger = getLogger("hub:proxy:file-download");
+
+// assumes request has already been authenticated!
+
+export async function handleFileDownload(req, res, url, project_id) {
+  logger.debug("handling the request via conat file streaming", url);
+  const i = url.indexOf("files/");
+  const compute_server_id = req.query.id ?? 0;
+  let j = url.lastIndexOf("?");
+  if (j == -1) {
+    j = url.length;
+  }
+  const path = decodeURIComponent(url.slice(i + "files/".length, j));
+  logger.debug("conat: get file", { project_id, path, compute_server_id, url });
+  const fileName = path_split(path).tail;
+  const contentType = mime.lookup(fileName);
+  if (req.query.download != null || DANGEROUS_CONTENT_TYPE.has(contentType)) {
+    const fileNameEncoded = encodeURIComponent(fileName)
+      .replace(/['()]/g, escape)
+      .replace(/\*/g, "%2A");
+    res.setHeader(
+      "Content-disposition",
+      `attachment; filename*=UTF-8''${fileNameEncoded}`,
+    );
+  }
+  res.setHeader("Content-type", contentType);
+
+  let headersSent = false;
+  res.on("finish", () => {
+    headersSent = true;
+  });
+  try {
+    for await (const chunk of await readProjectFile({
+      project_id,
+      compute_server_id,
+      path,
+      // allow a long download time (1 hour), since files can be large and
+      // networks can be slow.
+      maxWait: 1000 * 60 * 60,
+    })) {
+      if (res.writableEnded || res.destroyed) {
+        break;
+      }
+      if (!res.write(chunk)) {
+        await once(res, "drain");
+      }
+    }
+    res.end();
+  } catch (err) {
+    logger.debug(
+      "ERROR streaming file",
+      { project_id, compute_server_id, path },
+      err,
+    );
+    if (!headersSent) {
+      res.statusCode = 500;
+      res.end("Error reading file.");
+    } else {
+      // Data sent, forcibly kill the connection
+      res.destroy(err);
+    }
+  }
+}

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

@@ -10,13 +10,8 @@ import { stripBasePath } from "./util";
 import { ProjectControlFunction } from "@cocalc/server/projects/control";
 import siteUrl from "@cocalc/database/settings/site-url";
 import { parseReq } from "./parse";
-import { readFile as readProjectFile } from "@cocalc/conat/files/read";
-import { path_split } from "@cocalc/util/misc";
-import { once } from "@cocalc/util/async-utils";
 import hasAccess from "./check-for-access-to-project";
-import mime from "mime-types";
-
-const DANGEROUS_CONTENT_TYPE = new Set(["image/svg+xml" /*, "text/html"*/]);
+import { handleFileDownload } from "./file-download";
 
 const logger = getLogger("proxy:handle-request");
 
@@ -84,7 +79,6 @@ export default function init({ projectControl, isPersonal }: Options) {
     // TODO: parseReq is called again in getTarget so need to refactor...
     const { type, project_id } = parsed;
     if (type == "files") {
-      dbg("handling the request via conat file streaming");
       if (
         !(await hasAccess({
           project_id,
@@ -96,43 +90,7 @@ export default function init({ projectControl, isPersonal }: Options) {
       ) {
         throw Error(`user does not have read access to project`);
       }
-      const i = url.indexOf("files/");
-      const compute_server_id = req.query.id ?? 0;
-      let j = url.lastIndexOf("?");
-      if (j == -1) {
-        j = url.length;
-      }
-      const path = decodeURIComponent(url.slice(i + "files/".length, j));
-      dbg("conat: get file", { project_id, path, compute_server_id, url });
-      const fileName = path_split(path).tail;
-      const contentType = mime.lookup(fileName);
-      if (
-        req.query.download != null ||
-        DANGEROUS_CONTENT_TYPE.has(contentType)
-      ) {
-        const fileNameEncoded = encodeURIComponent(fileName)
-          .replace(/['()]/g, escape)
-          .replace(/\*/g, "%2A");
-        res.setHeader(
-          "Content-disposition",
-          `attachment; filename*=UTF-8''${fileNameEncoded}`,
-        );
-      }
-      res.setHeader("Content-type", contentType);
-      for await (const chunk of await readProjectFile({
-        project_id,
-        compute_server_id,
-        path,
-        // allow a long download time (1 hour), since files can be large and
-        // networks can be slow.
-        maxWait: 1000 * 60 * 60,
-      })) {
-        if (!res.write(chunk)) {
-          // backpressure -- wait for it to resolve
-          await once(res, "drain");
-        }
-      }
-      res.end();
+      await handleFileDownload(req, res, url, project_id);
       return;
     }
 
@@ -182,8 +140,14 @@ export default function init({ projectControl, isPersonal }: Options) {
       await handleProxyRequest(req, res);
     } catch (err) {
       const msg = `WARNING: error proxying request ${req.url} -- ${err}`;
-      res.writeHead(426, { "Content-Type": "text/html" });
-      res.end(msg);
+      try {
+        // this will fail if handleProxyRequest already wrote a header, so we
+        // try/catch it.
+        res.writeHead(500, { "Content-Type": "text/html" });
+      } catch {}
+      try {
+        res.end(msg);
+      } catch {}
       // Not something to log as an error -- just debug; it's normal for it to happen, e.g., when
       // a project isn't running.
       logger.debug(msg);

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

@@ -2,7 +2,6 @@
 The main hub express app.
 */
 
-import compression from "compression";
 import cookieParser from "cookie-parser";
 import express from "express";
 import ms from "ms";
@@ -33,6 +32,9 @@ import basePath from "@cocalc/backend/base-path";
 import { initConatServer } from "@cocalc/server/conat/socketio";
 import { conatSocketioCount } from "@cocalc/backend/data";
 
+// NOTE: we are not using compression because that interferes with streaming file download,
+// and could be generally confusing.
+
 // Used for longterm caching of files. This should be in units of seconds.
 const MAX_AGE = Math.round(ms("10 days") / 1000);
 const SHORT_AGE = Math.round(ms("10 seconds") / 1000);
@@ -80,12 +82,6 @@ export default async function init(opts: Options): Promise<{
     app.use(vhostShare());
   }
 
-  // Enable compression, as suggested by
-  //   http://expressjs.com/en/advanced/best-practice-performance.html#use-gzip-compression
-  // NOTE "Express runs everything in order" --
-  // https://github.com/expressjs/compression/issues/35#issuecomment-77076170
-  app.use(compression());
-
   app.use(cookieParser());
 
   // Install custom middleware to track response time metrics via prometheus