Skip to content

Commit

Permalink
Revert "Bun.serve: error: pass Request parameter when available (#9310)…
Browse files Browse the repository at this point in the history 
…" (#9332)

This reverts commit b92d985.
  • Loading branch information
@Jarred-Sumner
Jarred-Sumner authored Mar 9, 2024
1 parent cd32083 commit 79e77f1
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 132 deletions.
2 changes: 1 addition & 1 deletion docs/api/http.md
View file
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
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
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
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

0 comments on commit 79e77f1

Please sign in to comment.