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.

Sign up for GitHub

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

Revert "Bun.serve: error: pass Request parameter when available" #9332

Merged
merged 1 commit into from
Mar 9, 2024
Merged

Revert "Bun.serve: error: pass Request parameter when available" #9332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion docs/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ Bun.serve({
fetch(req) {
throw new Error("woops!");
},
error(error: Error, req: Request | null) {
error(error) {
return new Response(`<pre>${error}\n${error.stack}</pre>`, {
headers: {
"Content-Type": "text/html",
Expand Down
2 changes: 1 addition & 1 deletion packages/bun-types/bun.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2194,7 +2194,7 @@ declare module "bun" {
*/
development?: boolean;

error?: (this: Server, error: ErrorLike, request: Request | null) => Response | Promise<Response>;
error?: (this: Server, request: ErrorLike) => Response | Promise<Response> | undefined | Promise<undefined>;

/**
* Uniquely identify a server instance with an ID
Expand Down
111 changes: 50 additions & 61 deletions src/bun.js/api/server.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1373,7 +1373,6 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
if (!has_responded)
ctx.runErrorHandler(
value,
JSValue.jsNull(),
);

if (ctx.flags.aborted) {
Expand Down Expand Up @@ -1999,20 +1998,18 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
file.pathlike.fd
else switch (bun.sys.open(file.pathlike.path.sliceZ(&file_buf), std.os.O.RDONLY | std.os.O.NONBLOCK | std.os.O.CLOEXEC, 0)) {
.result => |_fd| _fd,
.err => |err| return this.runErrorHandler(
err.withPath(file.pathlike.path.slice()).toSystemError().toErrorInstance(this.server.globalThis),
JSValue.jsNull(),
),
.err => |err| return this.runErrorHandler(err.withPath(file.pathlike.path.slice()).toSystemError().toErrorInstance(
this.server.globalThis,
)),
};

// stat only blocks if the target is a file descriptor
const stat: bun.Stat = switch (bun.sys.fstat(fd)) {
.result => |result| result,
.err => |err| {
this.runErrorHandler(
err.withPathLike(file.pathlike).toSystemError().toErrorInstance(this.server.globalThis),
JSValue.jsNull(),
);
this.runErrorHandler(err.withPathLike(file.pathlike).toSystemError().toErrorInstance(
this.server.globalThis,
));
if (auto_close) {
_ = bun.sys.close(fd);
}
Expand All @@ -2032,10 +2029,9 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
};
var sys = err.withPathLike(file.pathlike).toSystemError();
sys.message = bun.String.static("MacOS does not support sending non-regular files");
this.runErrorHandler(
sys.toErrorInstance(this.server.globalThis),
JSValue.jsNull(),
);
this.runErrorHandler(sys.toErrorInstance(
this.server.globalThis,
));
return;
}
}
Expand All @@ -2052,10 +2048,9 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
};
var sys = err.withPathLike(file.pathlike).toSystemError();
sys.message = bun.String.static("File must be regular or FIFO");
this.runErrorHandler(
sys.toErrorInstance(this.server.globalThis),
JSValue.jsNull(),
);
this.runErrorHandler(sys.toErrorInstance(
this.server.globalThis,
));
return;
}
}
Expand Down Expand Up @@ -2132,10 +2127,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
}

if (result == .err) {
this.runErrorHandler(
result.err.toErrorInstance(this.server.globalThis),
JSValue.jsNull(),
);
this.runErrorHandler(result.err.toErrorInstance(this.server.globalThis));
return;
}

Expand Down Expand Up @@ -2459,7 +2451,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
}

if (response_value.toError()) |err_value| {
ctx.runErrorHandler(err_value, request_value);
ctx.runErrorHandler(err_value);
return;
}

Expand Down Expand Up @@ -2674,10 +2666,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
this.finalizeForAbort();
return;
}
this.runErrorHandler(
err,
JSC.JSValue.jsNull(),
);
this.runErrorHandler(err);
return;
},
// .InlineBlob,
Expand Down Expand Up @@ -2709,10 +2698,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
.message = bun.String.static("Stream already used, please create a new one"),
};
stream.value.unprotect();
this.runErrorHandler(
err.toErrorInstance(this.server.globalThis),
JSC.JSValue.jsNull(),
);
this.runErrorHandler(err.toErrorInstance(this.server.globalThis));
return;
}

Expand Down Expand Up @@ -2896,9 +2882,8 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
pub fn runErrorHandler(
this: *RequestContext,
value: JSC.JSValue,
request_value: JSValue,
) void {
runErrorHandlerWithStatusCode(this, value, 500, request_value);
runErrorHandlerWithStatusCode(this, value, 500);
}

const PathnameFormatter = struct {
Expand Down Expand Up @@ -2959,20 +2944,14 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
this: *RequestContext,
value: JSC.JSValue,
status: u16,
request_value: JSValue,
) void {
JSC.markBinding(@src());
if (!this.server.config.onError.isEmpty() and !this.flags.has_called_error_handler) {
this.flags.has_called_error_handler = true;

if (Environment.allow_assert) std.debug.assert(request_value != .zero);
const result = this.server.config.onError.callWithThis(
this.server.globalThis,
this.server.thisObject,
&.{
value,
request_value,
},
&.{value},
);
defer result.ensureStillAlive();
if (!result.isEmptyOrUndefinedOrNull()) {
Expand Down Expand Up @@ -3065,12 +3044,11 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
this: *RequestContext,
value: JSC.JSValue,
status: u16,
request_value: JSValue,
) void {
JSC.markBinding(@src());
if (this.resp == null or this.resp.?.hasResponded()) return;

runErrorHandlerWithStatusCodeDontCheckResponded(this, value, status, request_value);
runErrorHandlerWithStatusCodeDontCheckResponded(this, value, status);
}

pub fn renderMetadata(this: *RequestContext) void {
Expand Down Expand Up @@ -3366,24 +3344,6 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
return (this.resp orelse return null).getRemoteSocketInfo();
}

pub fn toRequestObject(this: *RequestContext, globalThis: *JSGlobalObject) !*JSC.WebCore.Request {
if (Environment.allow_assert) std.debug.assert(this.signal == null);
this.signal = JSC.WebCore.AbortSignal.new(globalThis);

if (Environment.allow_assert) std.debug.assert(this.request_body == null);
this.request_body = JSC.WebCore.InitRequestBodyValue(.{ .Null = {} }) catch unreachable;

const request_object = try this.allocator.create(JSC.WebCore.Request);
request_object.* = .{
.method = this.method,
.request_context = AnyRequestContext.init(this),
.https = ssl_enabled,
.signal = this.signal.?.ref(),
.body = this.request_body.?.ref(),
};
return request_object;
}

pub const Export = shim.exportFunctions(.{
.onResolve = onResolve,
.onReject = onReject,
Expand Down Expand Up @@ -5899,7 +5859,20 @@ pub fn NewServer(comptime NamespaceType: type, comptime ssl_enabled_: bool, comp
var ctx = this.request_pool_allocator.tryGet() catch @panic("ran out of memory");
ctx.create(this, req, resp);
this.vm.jsc.reportExtraMemory(@sizeOf(RequestContext));
const request_object = ctx.toRequestObject(this.globalThis) catch unreachable;
var request_object = this.allocator.create(JSC.WebCore.Request) catch unreachable;
var body = JSC.WebCore.InitRequestBodyValue(.{ .Null = {} }) catch unreachable;

ctx.request_body = body;
var signal = JSC.WebCore.AbortSignal.new(this.globalThis);
ctx.signal = signal;

request_object.* = .{
.method = ctx.method,
.request_context = AnyRequestContext.init(ctx),
.https = ssl_enabled,
.signal = signal.ref(),
.body = body.ref(),
};

if (comptime debug_mode) {
ctx.flags.is_web_browser_navigation = brk: {
Expand Down Expand Up @@ -5957,8 +5930,10 @@ pub fn NewServer(comptime NamespaceType: type, comptime ssl_enabled_: bool, comp
request_object.toJS(this.globalThis),
this.thisObject,
};

const request_value = args[0];
request_value.ensureStillAlive();

const response_value = this.config.onRequest.callWithThis(this.globalThis, this.thisObject, &args);
defer {
// uWS request will not live longer than this function
Expand Down Expand Up @@ -6002,7 +5977,21 @@ pub fn NewServer(comptime NamespaceType: type, comptime ssl_enabled_: bool, comp
req.setYield(false);
var ctx = this.request_pool_allocator.tryGet() catch @panic("ran out of memory");
ctx.create(this, req, resp);
const request_object = ctx.toRequestObject(this.globalThis) catch unreachable;
var request_object = this.allocator.create(JSC.WebCore.Request) catch unreachable;
var body = JSC.WebCore.InitRequestBodyValue(.{ .Null = {} }) catch unreachable;

ctx.request_body = body;
var signal = JSC.WebCore.AbortSignal.new(this.globalThis);
ctx.signal = signal;

request_object.* = .{
.method = ctx.method,
.request_context = AnyRequestContext.init(ctx),
.upgrader = ctx,
.https = ssl_enabled,
.signal = signal.ref(),
.body = body.ref(),
};
ctx.upgrade_context = upgrade_ctx;

// We keep the Request object alive for the duration of the request so that we can remove the pointer to the UWS request object.
Expand Down
69 changes: 0 additions & 69 deletions test/js/bun/http/bun-server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,6 @@ describe("Server", () => {
server.stop(true);
}
});

test("abort signal on server with stream", async () => {
{
let signalOnServer = false;
Expand Down Expand Up @@ -434,74 +433,6 @@ test("unref keeps process alive for ongoing connections", async () => {
expect([path.join(import.meta.dir, "unref-fixture-2.ts")]).toRun();
});

describe("Bun.serve error handling", () => {
test("supports error handling", async () => {
const server = Bun.serve({
port: 0,
fetch(req) {
throw new Error("woops!");
},
error(error) {
return new Response(`${error.message}`);
},
});

const response = await fetch(`http://${server.hostname}:${server.port}`);
expect(await response.text()).toBe("woops!");
server.stop(true);
});

test("supports reading the Request in error handling", async () => {
const server = Bun.serve({
port: 0,
fetch(req) {
throw new Error("woops!");
},
error(error, req) {
if (req === null) return new Response(`${error.message}`);
return new Response(`${error.message}\n${req.method}`);
},
});

const response = await fetch(`http://${server.hostname}:${server.port}`);
expect(await response.text()).toBe("woops!\nGET");
server.stop(true);
});

test("the request headers survive", async () => {
const server = Bun.serve({
port: 0,
fetch(req) {
throw new Error("woops!");
},
error(error, req) {
return new Response(`${req.headers.get("x-foo")}`);
},
});

const response = await fetch(`http://${server.hostname}:${server.port}`, { headers: { "x-foo": "1" } });
expect(await response.text()).toBe("1");
server.stop(true);
});

test("the request url survives", async () => {
const server = Bun.serve({
port: 0,
fetch(req) {
throw new Error("woops!");
},
error(error, req) {
return new Response(`${req.url}`);
},
});

const url = `http://${server.hostname}:${server.port}/`;
const response = await fetch(url);
expect(await response.text()).toBe(url);
server.stop(true);
});
});

test("Bun does not crash when given invalid config", async () => {
const server1 = Bun.serve({
fetch(request, server) {
Expand Down
Loading