oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
74.16k stars 2.77k forks source link

hono serveStatic panic / hard crash (without bun.report) if with an async middleware (redis in this case) #12717

Open 0xFA11 opened 3 months ago

0xFA11 commented 3 months ago

How can we reproduce the crash?

I'll list 3 very similar code snippets, 1 crashes and other 2 doesn't crash.

  1. This is OK
    app.get(
    // this is OK
    "/public/*",
    serveStatic({ root: "./" }),
    );
  2. This is also OK
    app.get(
    // this is OK
    "/public/*",
    async (c, next) => {
        await new Redis().ping();
        await new Promise((r) => setTimeout(r, 1));
        await next();
    },
    serveStatic({ root: "./" }),
    );
  3. This panics and crashes
    app.get(
    // [43058] pas panic: deallocation did fail at 0x16bcd9200: Large heap did not find object
    "/public/*",
    async (c, next) => {
        await new Redis().ping();
        await next();
    },
    serveStatic({ root: "./" }),
    );

hono 4.5.1 bun 1.1.20

Interestingly, without fix #9751 applied, in bun 1.0.36, it works just fine.

Another interesting details is that 1ms timeout promise fixes it. After looking at #9751 a bit more, I can see allocator changes which makes me think that things are potentially double freed but that's just my hypothesis, I don't have any proof (didn't debug native code myself yet).

A final interesting detail is that crash happens after the response is fully sent to the client (see log output below).

Relevant log output

<-- GET /public/bla.txt
  --> GET /public/bla.txt 200 9ms
[43326] pas panic: deallocation did fail at 0x16d256d00: Large heap did not find object
zsh: trace trap  bun run index.ts```

Stack Trace (bun.report)

Unfortunately, there is no bun.report because it's a hard crash at runtime, it doesn't generate a crash report.

Jarred-Sumner commented 3 months ago

@ThusSpokeNomad any chance you could give us a reproduction without Hono?

0xFA11 commented 3 months ago

@ThusSpokeNomad any chance you could give us a reproduction without Hono?

what would that HTTP framework be? Elysia? IDK what would that actually look like...

this specific reproduction is happening with even the simplest hono application — which is highly popular.

literally, follow these simple getting started steps https://hono.dev/docs/getting-started/bun and plug iovalkey or ioredis as a middleware to a simple route like I did above.

Jarred-Sumner commented 3 months ago

Use Bun.serve directly, which all these frameworks wrap

0xFA11 commented 3 months ago

Use Bun.serve directly, which all these frameworks wrap

here you go @Jarred-Sumner :

import { serve } from "bun";
import Redis from "iovalkey";

serve({
    async fetch(req) {
        const redis = new Redis("redis://localhost");
        await redis.ping();

        const path = new URL(req.url).pathname;
        const file = Bun.file("." + path);
        return new Response(file);
    },
});
$ bun run index.ts
[49828] pas panic: deallocation did fail at 0x16b46ed00: Large heap did not find object
zsh: trace trap  bun run index.ts
Jarred-Sumner commented 3 months ago

@ThusSpokeNomad What operating system and architecture? Not able to reproduce this on macOS.

0xFA11 commented 3 months ago

@Jarred-Sumner M1 Max, macOS 14.5 23F79 arm64, Bun 1.1.20 I will try to reproduce this inside a linux docker container.

Jarred-Sumner commented 3 months ago

I'm able to reproduce this now after some fiddling.

Jarred-Sumner commented 3 months ago

Note that the original issue is fixed in the canary build. It no longer crashes. There is a different bug when using new Response(Bun.file(path)) that needs to be addressed.

0xFA11 commented 3 months ago

@Jarred-Sumner

unfortunately, the original issue still persists for me on canary with hono, see the snippet below:

import { serve } from "bun";
import { Hono } from "hono";
import { serveStatic } from "hono/bun";
import Redis from "iovalkey";

/* serve({
    async fetch(req) {
        const redis = new Redis("redis://localhost");
        await redis.ping();

        const path = new URL(req.url).pathname;
        const file = Bun.file("." + path);
        return new Response(file);
    },
}); */

const app = new Hono();

app.use(
    "/public/*",
    async (c, next) => {
        const redis = new Redis("redis://localhost");
        await redis.ping();
        await next();
    },
    serveStatic({ root: "./" })
);

serve({
    fetch: app.fetch,
});

other details:

$ bun upgrade --canary
[2.22s] Upgraded.

Welcome to Bun's latest canary build!

Report any bugs:

    https://github.com/oven-sh/bun/issues

Changelog:

    https://github.com/oven-sh/bun/compare/ae19489250ab74ee3ea7d5b50fca5d7d2b4e6d66...main
$ bun --version
1.1.21
0xFA11 commented 3 months ago

I haven't checked but I assume serveStatic(...) would eventually boil down to new Response(Bun.file(path)) as well, hence the issue still persists. I'll keep an eye on this issue if/when new Response(Bun.file(path)) thing gets fixed, this hopefully should also get fixed too (fingers-crossed)

0xFA11 commented 3 months ago

Hey @Jarred-Sumner, sorry for pinging and disturbing. I'm just curious if there's any updates or plans to address this issue any time soon (or is it tracked somewhere else and/or part of a wider plan or refactor)?