Skip to content

Commit

Permalink
Fix memory leak in new Request(request) (oven-sh#12387)
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarred-Sumner authored Jul 6, 2024
1 parent cd97c21 commit 41a5ebe
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 25 deletions.
44 changes: 19 additions & 25 deletions src/bun.js/webcore/request.zig
Original file line number Diff line number Diff line change
Expand Up @@ -476,19 +476,28 @@ pub const Request = struct {
globalThis: *JSC.JSGlobalObject,
arguments: []const JSC.JSValue,
) ?Request {
var success = false;
const body = InitRequestBodyValue(.{ .Null = {} }) catch {
return null;
};
var req = Request{
.body = InitRequestBodyValue(.{ .Null = {} }) catch {
return null;
},
.body = body,
};
defer {
if (!success) {
req.finalizeWithoutDeinit();
_ = req.body.unref();
}
if (req.body != body) {
_ = body.unref();
}
}

if (arguments.len == 0) {
globalThis.throw("Failed to construct 'Request': 1 argument required, but only 0 present.", .{});
_ = req.body.unref();
return null;
} else if (arguments[0].isEmptyOrUndefinedOrNull() or !arguments[0].isCell()) {
globalThis.throw("Failed to construct 'Request': expected non-empty string or object, got undefined", .{});
_ = req.body.unref();
return null;
}

Expand All @@ -504,8 +513,6 @@ pub const Request = struct {

if (is_first_argument_a_url) {
const str = bun.String.tryFromJS(arguments[0], globalThis) orelse {
req.finalizeWithoutDeinit();
_ = req.body.unref();
return null;
};
req.url = str;
Expand All @@ -514,7 +521,6 @@ pub const Request = struct {
fields.insert(.url);
} else if (!url_or_object_type.isObject()) {
globalThis.throw("Failed to construct 'Request': expected non-empty string or object", .{});
_ = req.body.unref();
return null;
}

Expand All @@ -536,6 +542,7 @@ pub const Request = struct {
if (value.asDirect(Request)) |request| {
if (values_to_try.len == 1) {
request.cloneInto(&req, globalThis.allocator(), globalThis, fields.contains(.url));
success = true;
return req;
}

Expand Down Expand Up @@ -597,13 +604,9 @@ pub const Request = struct {
if (!fields.contains(.body)) {
if (value.fastGet(globalThis, .body)) |body_| {
fields.insert(.body);
if (Body.Value.fromJS(globalThis, body_)) |body| {
req.body.value = body;
} else {
req.finalizeWithoutDeinit();
_ = req.body.unref();
req.body.value = Body.Value.fromJS(globalThis, body_) orelse {
return null;
}
};
}
}

Expand All @@ -617,11 +620,7 @@ pub const Request = struct {
} else if (@intFromEnum(value) == @intFromEnum(values_to_try[values_to_try.len - 1]) and !is_first_argument_a_url and
value.implementsToString(globalThis))
{
const str = bun.String.tryFromJS(value, globalThis) orelse {
req.finalizeWithoutDeinit();
_ = req.body.unref();
return null;
};
const str = bun.String.tryFromJS(value, globalThis) orelse return null;
req.url = str;
if (!req.url.isEmpty())
fields.insert(.url);
Expand All @@ -637,8 +636,6 @@ pub const Request = struct {
req.signal = signal.ref();
} else {
globalThis.throw("Failed to construct 'Request': signal is not of type AbortSignal.", .{});
req.finalizeWithoutDeinit();
_ = req.body.unref();
return null;
}
}
Expand Down Expand Up @@ -667,8 +664,6 @@ pub const Request = struct {
}
if (req.url.isEmpty()) {
globalThis.throw("Failed to construct 'Request': url is required.", .{});
req.finalizeWithoutDeinit();
_ = req.body.unref();
return null;
}

Expand All @@ -679,8 +674,6 @@ pub const Request = struct {
globalThis.throw("Failed to construct 'Request': Invalid URL \"{}\"", .{
req.url,
});
req.finalizeWithoutDeinit();
_ = req.body.unref();
return null;
}

Expand All @@ -702,6 +695,7 @@ pub const Request = struct {
}

req.calculateEstimatedByteSize();
success = true;

return req;
}
Expand Down
103 changes: 103 additions & 0 deletions test/js/web/request/request-clone-leak.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import { test, expect } from "bun:test";

const constructorArgs = [
[
new Request("http://foo/", {
body: "ahoyhoy",
method: "POST",
}),
],
[
"http://foo/",
{
body: "ahoyhoy",
method: "POST",
},
],
[
new URL("http://foo/"),
{
body: "ahoyhoy",
method: "POST",
},
],
[
new Request("http://foo/", {
body: "ahoyhoy",
method: "POST",
headers: {
"test-header": "value",
},
}),
],
[
"http://foo/",
{
body: "ahoyhoy",
method: "POST",
headers: {
"test-header": "value",
},
},
],
[
new URL("http://foo/"),
{
body: "ahoyhoy",
method: "POST",
headers: {
"test-header": "value",
},
},
],
];
for (let i = 0; i < constructorArgs.length; i++) {
const args = constructorArgs[i];
test("new Request(test #" + i + ")", () => {
Bun.gc(true);

for (let i = 0; i < 1000; i++) {
new Request(...args);
}

Bun.gc(true);
const baseline = (process.memoryUsage.rss() / 1024 / 1024) | 0;
for (let i = 0; i < 2000; i++) {
for (let j = 0; j < 500; j++) {
new Request(...args);
}
Bun.gc();
}
Bun.gc(true);

const memory = (process.memoryUsage.rss() / 1024 / 1024) | 0;
const delta = Math.max(memory, baseline) - Math.min(baseline, memory);
console.log("RSS delta: ", delta, "MB");
expect(delta).toBeLessThan(30);
});

test("request.clone(test #" + i + ")", () => {
Bun.gc(true);

for (let i = 0; i < 1000; i++) {
const request = new Request(...args);
request.clone();
}

Bun.gc(true);
const baseline = (process.memoryUsage.rss() / 1024 / 1024) | 0;
for (let i = 0; i < 2000; i++) {
for (let j = 0; j < 500; j++) {
const request = new Request(...args);
request.clone();
}
Bun.gc();
}
Bun.gc(true);

const memory = (process.memoryUsage.rss() / 1024 / 1024) | 0;
const delta = Math.max(memory, baseline) - Math.min(baseline, memory);
console.log("RSS delta: ", delta, "MB");
expect(delta).toBeLessThan(30);
});
}

0 comments on commit 41a5ebe

Please sign in to comment.