Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consuming and cancelling the IncomingMessage stream crashes the server. #185

Open
Markyiptw opened this issue Aug 12, 2024 · 2 comments
Open

Comments

@Markyiptw
Copy link

Markyiptw commented Aug 12, 2024

I have an endpoint for accepting huge files, so instead of loading them at once into memory, I choose to consume incoming as stream and process it with busboy. The happy path works properly, but the server crashes when I start to do stream error handling:

import { pipeline } from "stream/promises";
import { Busboy } from "@fastify/busboy";
import { HttpBindings } from "@hono/node-server";
import { Hono } from "hono";

const app = new Hono<{ Bindings: HttpBindings }>();

app.post("/", async (c) => {
  const { incoming, outgoing } = c.env;
  const busboy = Busboy({
    headers: incoming.headers,
  });
  busboy.on("file", async (fieldname, file, filename, encoding, mimetype) => {
    busboy.emit("error", new Error("Something bad happened"));
  }
  try {
    await pipeline(incoming, busboy);
    return c.html("never reached, as stream/promises/pipeline will throw on error event");
  } catch (err: Error) {
    outgoing.writeHead(500, { Connection: "close" });
    outgoing.end(err.message);
    return RESPONSE_ALREADY_SENT;
  }
)

The whole server crashes with the above code after responding to the client. The error is caused by https://github.com/nodejs/undici/blob/main/lib/web/fetch/body.js#L191, which basically happen when a disturbed stream is being used to initial a request body (cancelled is considered as disturbed in this case). And since those node apis are out of our control, I wonder if we should

  • wrap newRequestFromIncoming with some try catch logic?
  • don't initialize the request when RESPONSE_ALREADY_SENT is returned? or at least make req.body empty?
  • most importantly, we shouldn't let the whole app crash in any case, app.onError should have caught the error.

Thank you!

@ManuelFernando
Copy link

@Markyiptw @yusukebe this snippet of code works but when this endpoint is after a middleware which has await operation. For some unknown reason the requested appears streamed and Busboy is unable to read anything

@ManuelFernando
Copy link

the problem came with incoming, instead await pipeline(c.req.raw.body, busboy); resolve the problem

@Markyiptw @yusukebe this snippet of code works but when this endpoint is after a middleware which has await operation. For some unknown reason the requested appears streamed and Busboy is unable to read anything

the problem came with incoming, instead await pipeline(c.req.raw.body, busboy); resolve the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants