From 510a87b0ce69b9538b2265e59bf70a700f3b815d Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Thu, 21 Nov 2024 16:18:33 -0800 Subject: [PATCH 1/5] fix redirect with empty streams --- src/bun.js/api/server.zig | 6 ++++ test/js/bun/http/bun-server.test.ts | 44 ++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/bun.js/api/server.zig b/src/bun.js/api/server.zig index 2946d6a5143116..86a011a901dc26 100644 --- a/src/bun.js/api/server.zig +++ b/src/bun.js/api/server.zig @@ -2911,6 +2911,12 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp signal.clear(); assert(signal.isDead()); + // we need to render metadata before assignToStream because the stream can call res.end + // and this would auto write an 200 status + if (!this.flags.has_written_status) { + this.renderMetadata(); + } + // We are already corked! const assignment_result: JSValue = ResponseStream.JSSink.assignToStream( globalThis, diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index de430f36e39b1c..a7639bdc870658 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -317,8 +317,7 @@ describe("Server", () => { } }); - - test('server should return a body for a OPTIONS Request', async () => { + test("server should return a body for a OPTIONS Request", async () => { using server = Bun.serve({ port: 0, fetch(req) { @@ -327,16 +326,17 @@ describe("Server", () => { }); { const url = `http://${server.hostname}:${server.port}/`; - const response = await fetch(new Request(url, { - method: 'OPTIONS', - })); + const response = await fetch( + new Request(url, { + method: "OPTIONS", + }), + ); expect(await response.text()).toBe("Hello World!"); expect(response.status).toBe(200); expect(response.url).toBe(url); } }); - test("abort signal on server with stream", async () => { { let signalOnServer = false; @@ -456,7 +456,7 @@ describe("Server", () => { env: bunEnv, stderr: "pipe", }); - expect(stderr.toString('utf-8')).toBeEmpty(); + expect(stderr.toString("utf-8")).toBeEmpty(); expect(exitCode).toBe(0); }); }); @@ -768,3 +768,33 @@ test.skip("should be able to stream huge amounts of data", async () => { expect(written).toBe(CONTENT_LENGTH); expect(received).toBe(CONTENT_LENGTH); }, 30_000); + +test("should be able to redirect when using empty streams #15320", async () => { + using server = Bun.serve({ + port: 0, + websocket: void 0, + async fetch(req, server2) { + const url = new URL(req.url); + if (url.pathname === "/redirect") { + const emptyStream = new ReadableStream({ + start(controller) { + // Immediately close the stream to make it empty + controller.close(); + }, + }); + + return new Response(emptyStream, { + status: 307, + headers: { + location: "/", + }, + }); + } + + return new Response("Hello, World"); + }, + }); + + const response = await fetch(`http://localhost:${server.port}/redirect`); + expect(await response.text()).toBe("Hello, World"); +}); From ca26541cd2607bd427e7aac27ae73751ce65764f Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Thu, 21 Nov 2024 17:14:48 -0800 Subject: [PATCH 2/5] fix test --- test/js/web/fetch/body-stream.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/js/web/fetch/body-stream.test.ts b/test/js/web/fetch/body-stream.test.ts index f42de34e02fc06..1b9bc335962c38 100644 --- a/test/js/web/fetch/body-stream.test.ts +++ b/test/js/web/fetch/body-stream.test.ts @@ -34,9 +34,8 @@ for (let doClone of [true, false]) { }, async url => { called = true; - expect(await fetch(url).then(res => res.text())).toContain( - "Welcome to Bun! To get started, return a Response object.", - ); + // if we can flush it will be "hey" otherwise will be empty + expect(await fetch(url).then(res => res.text())).toBeOneOf(["hey", ""]); }, ); From 5dda8788bcd29a55e9de377327005ac0eca9c448 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Fri, 22 Nov 2024 15:59:22 -0800 Subject: [PATCH 3/5] fix thi --- src/bun.js/webcore/response.zig | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bun.js/webcore/response.zig b/src/bun.js/webcore/response.zig index 46e8069f049553..1044f22a183fae 100644 --- a/src/bun.js/webcore/response.zig +++ b/src/bun.js/webcore/response.zig @@ -1785,12 +1785,13 @@ pub const Fetch = struct { } pub fn callback(task: *FetchTasklet, async_http: *http.AsyncHTTP, result: http.HTTPClientResult) void { - task.mutex.lock(); - defer task.mutex.unlock(); const is_done = !result.has_more; // we are done with the http client so we can deref our side defer if (is_done) task.deref(); + task.mutex.lock(); + // we need to unlock before task.deref(); + defer task.mutex.unlock(); task.http.?.* = async_http.*; task.http.?.response_buffer = async_http.response_buffer; From fff5324079867a9afd8eb8526575152d52912c80 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Fri, 22 Nov 2024 17:05:18 -0800 Subject: [PATCH 4/5] always deinit from main thread --- src/bun.js/webcore/response.zig | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/bun.js/webcore/response.zig b/src/bun.js/webcore/response.zig index 1044f22a183fae..acf8a0513f463a 100644 --- a/src/bun.js/webcore/response.zig +++ b/src/bun.js/webcore/response.zig @@ -834,6 +834,18 @@ pub const Fetch = struct { } } + pub fn derefFromThread(this: *FetchTasklet) void { + const count = this.ref_count.fetchSub(1, .monotonic); + bun.debugAssert(count > 0); + + if (count == 1) { + // this is really unlikely to happen, but can happen + // lets make sure that we always call deinit from main thread + + this.javascript_vm.eventLoop().enqueueTaskConcurrent(JSC.ConcurrentTask.fromCallback(this, FetchTasklet.deinit)); + } + } + pub const HTTPRequestBody = union(enum) { AnyBlob: AnyBlob, Sendfile: http.Sendfile, @@ -918,7 +930,7 @@ pub const Fetch = struct { this.clearAbortSignal(); } - fn deinit(this: *FetchTasklet) void { + pub fn deinit(this: *FetchTasklet) void { log("deinit", .{}); bun.assert(this.ref_count.load(.monotonic) == 0); @@ -1785,9 +1797,11 @@ pub const Fetch = struct { } pub fn callback(task: *FetchTasklet, async_http: *http.AsyncHTTP, result: http.HTTPClientResult) void { + // at this point only this thread is accessing result to is no race condition const is_done = !result.has_more; // we are done with the http client so we can deref our side - defer if (is_done) task.deref(); + // this is a atomic operation and will enqueue a task to deinit on the main thread + defer if (is_done) task.derefFromThread(); task.mutex.lock(); // we need to unlock before task.deref(); From f0aaf6b6edf35c0f08fb6b4fb385bdb0df8eebf4 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Tue, 26 Nov 2024 14:12:36 -0800 Subject: [PATCH 5/5] revert --- src/bun.js/webcore/response.zig | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/bun.js/webcore/response.zig b/src/bun.js/webcore/response.zig index acf8a0513f463a..46e8069f049553 100644 --- a/src/bun.js/webcore/response.zig +++ b/src/bun.js/webcore/response.zig @@ -834,18 +834,6 @@ pub const Fetch = struct { } } - pub fn derefFromThread(this: *FetchTasklet) void { - const count = this.ref_count.fetchSub(1, .monotonic); - bun.debugAssert(count > 0); - - if (count == 1) { - // this is really unlikely to happen, but can happen - // lets make sure that we always call deinit from main thread - - this.javascript_vm.eventLoop().enqueueTaskConcurrent(JSC.ConcurrentTask.fromCallback(this, FetchTasklet.deinit)); - } - } - pub const HTTPRequestBody = union(enum) { AnyBlob: AnyBlob, Sendfile: http.Sendfile, @@ -930,7 +918,7 @@ pub const Fetch = struct { this.clearAbortSignal(); } - pub fn deinit(this: *FetchTasklet) void { + fn deinit(this: *FetchTasklet) void { log("deinit", .{}); bun.assert(this.ref_count.load(.monotonic) == 0); @@ -1797,15 +1785,12 @@ pub const Fetch = struct { } pub fn callback(task: *FetchTasklet, async_http: *http.AsyncHTTP, result: http.HTTPClientResult) void { - // at this point only this thread is accessing result to is no race condition + task.mutex.lock(); + defer task.mutex.unlock(); const is_done = !result.has_more; // we are done with the http client so we can deref our side - // this is a atomic operation and will enqueue a task to deinit on the main thread - defer if (is_done) task.derefFromThread(); + defer if (is_done) task.deref(); - task.mutex.lock(); - // we need to unlock before task.deref(); - defer task.mutex.unlock(); task.http.?.* = async_http.*; task.http.?.response_buffer = async_http.response_buffer;