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

feat: Add async support to Bun.serve onError #8233

Merged
merged 10 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion src/bun.js/api/server.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,8 @@ fn NewFlags(comptime debug_mode: bool) type {
response_protected: bool = false,
aborted: bool = false,
has_finalized: bun.DebugOnly(bool) = bun.DebugOnlyDefault(false),

is_error_promise_pending: bool = false,
};
}

Expand Down Expand Up @@ -1377,7 +1379,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
return;
}

if (!resp.hasResponded() and !ctx.flags.has_marked_pending) {
if (!resp.hasResponded() and !ctx.flags.has_marked_pending and !ctx.flags.is_error_promise_pending) {
ctx.renderMissing();
return;
}
Expand Down Expand Up @@ -2951,6 +2953,9 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
if (result.toError()) |err| {
this.finishRunningErrorHandler(err, status);
return;
} else if (result.asAnyPromise()) |promise| {
this.processOnErrorPromise(result, promise, value, status);
return;
} else if (result.as(Response)) |response| {
this.render(response);
return;
Expand All @@ -2961,6 +2966,75 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
this.finishRunningErrorHandler(value, status);
}

fn processOnErrorPromise(
ctx: *RequestContext,
promise_js: JSC.JSValue,
promise: JSC.AnyPromise,
value: JSC.JSValue,
status: u16,
) void {
var vm = ctx.server.vm;

switch (promise.status(vm.global.vm())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy of what RequestContext.onResponse is doing. I'm not sure if this is the correct way of handling the promise.

.Pending => {},
.Fulfilled => {
const fulfilled_value = promise.result(vm.global.vm());

// if you return a Response object or a Promise<Response>
// but you upgraded the connection to a WebSocket
// just ignore the Response object. It doesn't do anything.
// it's better to do that than to throw an error
if (ctx.didUpgradeWebSocket()) {
return;
}

var response = fulfilled_value.as(JSC.WebCore.Response) orelse {
ctx.finishRunningErrorHandler(value, status);
return;
};

ctx.response_jsvalue = fulfilled_value;
ctx.response_jsvalue.ensureStillAlive();
ctx.flags.response_protected = false;
ctx.response_ptr = response;

response.body.value.toBlobIfPossible();
switch (response.body.value) {
.Blob => |*blob| {
if (blob.needsToReadFile()) {
fulfilled_value.protect();
ctx.flags.response_protected = true;
}
},
.Locked => {
fulfilled_value.protect();
ctx.flags.response_protected = true;
},
else => {},
}
ctx.render(response);
return;
},
.Rejected => {
promise.setHandled(vm.global.vm());
ctx.finishRunningErrorHandler(promise.result(vm.global.vm()), status);
return;
},
}

// Promise is not fulfilled yet
{
ctx.flags.is_error_promise_pending = true;
ctx.pending_promises_for_abort += 1;
promise_js.then(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added new callbacks for the error resolve/reject but realized that using onResolve / onError is working fine due to the deduplication check in runErrorHandlerWithStatusCodeDontCheckResponded.

ctx.server.globalThis,
ctx,
RequestContext.onResolve,
RequestContext.onReject,
);
}
}

pub fn runErrorHandlerWithStatusCode(
this: *RequestContext,
value: JSC.JSValue,
Expand Down
53 changes: 53 additions & 0 deletions test/js/bun/http/serve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@
return new Response(
new ReadableStream({
pull(controller) {
throw new Error("TestPassed");

Check failure on line 239 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error: TestPassed

at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:239:25) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 239 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error: TestPassed

at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:239:25) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 239 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error: TestPassed

at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:239:25) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 239 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error: TestPassed

at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:239:25) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)
},
cancel(reason) {},
}),
Expand Down Expand Up @@ -275,7 +275,7 @@
async pull(controller) {
controller.enqueue("PASS");
controller.close();
throw new Error("FAIL");

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error: FAIL

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)

Check failure on line 278 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error: FAIL

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:278:25 at pull (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:275:40) at promiseInvokeOrNoopMethodNoCatch (:1:21) at promiseInvokeOrNoopMethod (:1:21) at readableStreamDefaultControllerCallPullIfNeeded (:1:21)
},
});
console.log("after constructing ReadableStream");
Expand Down Expand Up @@ -1350,6 +1350,59 @@

server.stop(true);
});

it("should support promise returned from error", async () => {
const server = Bun.serve({
port: 0,
fetch(req) {
throw new Error(req.url);
},
async error(e) {
if (e.message.endsWith("/async-fulfilled")) {
return new Response("OK");
}

if (e.message.endsWith("/async-rejected")) {
throw new Error("");

Check failure on line 1366 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-aarch64

error

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1366:15 at error (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1360:20)

Check failure on line 1366 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1366:15 at error (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1360:20)

Check failure on line 1366 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1366:15 at error (/home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1360:20)

Check failure on line 1366 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1366:15 at error (/Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1360:20)
}

if (e.message.endsWith("/async-rejected-pending")) {
await Bun.sleep(100);
throw new Error("");

Check failure on line 1371 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64-baseline

error

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1371:15

Check failure on line 1371 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests linux-x64

error

at /home/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1371:15

Check failure on line 1371 in test/js/bun/http/serve.test.ts

View workflow job for this annotation

GitHub Actions / Tests bun-darwin-x64

error

at /Users/runner/work/bun/bun/test/js/bun/http/serve.test.ts:1371:15
}

if (e.message.endsWith("/async-pending")) {
await Bun.sleep(100);
return new Response("OK");
}
},
});

{
const resp = await fetch(`http://${server.hostname}:${server.port}/async-fulfilled`);
expect(resp.status).toBe(200);
expect(await resp.text()).toBe("OK");
}

{
const resp = await fetch(`http://${server.hostname}:${server.port}/async-pending`);
expect(resp.status).toBe(200);
expect(await resp.text()).toBe("OK");
}

{
const resp = await fetch(`http://${server.hostname}:${server.port}/async-rejected`);
expect(resp.status).toBe(500);
}

{
const resp = await fetch(`http://${server.hostname}:${server.port}/async-rejected-pending`);
expect(resp.status).toBe(500);
}

server.stop(true);
});

if (process.platform === "linux")
it("should use correct error when using a root range port(#7187)", () => {
expect(() => {
Expand Down
Loading