From ed7741a662715c6175b94d4e2b16d5329a7502ab Mon Sep 17 00:00:00 2001 From: Meghan Denny Date: Fri, 6 Sep 2024 16:11:19 -0700 Subject: [PATCH] node:https: provide proper Agent definition (#11826) Co-authored-by: Jarred Sumner --- src/bun.js/bindings/ErrorCode.cpp | 41 ++++++++++---- src/bun.js/bindings/ErrorCode.h | 1 + src/bun.js/bindings/ErrorCode.ts | 4 +- src/js/internal/cluster/Worker.ts | 3 +- src/js/internal/errors.ts | 1 + src/js/internal/url.ts | 23 ++++++++ src/js/node/fs.ts | 3 +- src/js/node/http.ts | 87 ++++++++--------------------- src/js/node/https.ts | 48 +++++++++++++--- src/js/node/url.ts | 20 +------ src/js/node/zlib.ts | 6 +- test/js/node/http/node-http.test.ts | 75 ++++++++++++++++++++++--- 12 files changed, 191 insertions(+), 121 deletions(-) create mode 100644 src/js/internal/url.ts diff --git a/src/bun.js/bindings/ErrorCode.cpp b/src/bun.js/bindings/ErrorCode.cpp index 2191c4bd63a37..f5f0f8b552881 100644 --- a/src/bun.js/bindings/ErrorCode.cpp +++ b/src/bun.js/bindings/ErrorCode.cpp @@ -69,6 +69,15 @@ extern "C" JSC::EncodedJSValue Bun__ERR_IPC_CHANNEL_CLOSED(JSC::JSGlobalObject* // clang-format on +#define EXPECT_ARG_COUNT(count__) \ + do { \ + auto argCount = callFrame->argumentCount(); \ + if (argCount < count__) { \ + JSC::throwTypeError(globalObject, scope, "requires " #count__ " arguments"_s); \ + return {}; \ + } \ + } while (false) + namespace Bun { using namespace JSC; @@ -196,11 +205,8 @@ JSC_DEFINE_HOST_FUNCTION(jsFunction_ERR_INVALID_ARG_TYPE, (JSC::JSGlobalObject * JSC::VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto argCount = callFrame->argumentCount(); - if (argCount < 3) { - JSC::throwTypeError(globalObject, scope, "requires 3 arguments"_s); - return {}; - } + EXPECT_ARG_COUNT(3); + auto arg_name = callFrame->argument(0); auto expected_type = callFrame->argument(1); auto actual_value = callFrame->argument(2); @@ -246,11 +252,7 @@ JSC_DEFINE_HOST_FUNCTION(jsFunction_ERR_OUT_OF_RANGE, (JSC::JSGlobalObject * glo JSC::VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - auto argCount = callFrame->argumentCount(); - if (argCount < 3) { - JSC::throwTypeError(globalObject, scope, "requires 3 arguments"_s); - return {}; - } + EXPECT_ARG_COUNT(3); auto arg_name = callFrame->argument(0).toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); @@ -320,7 +322,7 @@ extern "C" JSC::EncodedJSValue Bun__ERR_MISSING_ARGS_static(JSC::JSGlobalObject* JSC::VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); - if (arg1 == 0) { + if (arg1 == nullptr) { JSC::throwTypeError(globalObject, scope, "requires at least 1 argument"_s); return {}; } @@ -365,6 +367,23 @@ JSC_DEFINE_HOST_FUNCTION(jsFunction_ERR_SOCKET_BAD_TYPE, (JSC::JSGlobalObject * return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_SOCKET_BAD_TYPE, "Bad socket type specified. Valid types are: udp4, udp6"_s)); } +JSC_DEFINE_HOST_FUNCTION(jsFunction_ERR_INVALID_PROTOCOL, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +{ + JSC::VM& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + EXPECT_ARG_COUNT(2); + + auto actual = callFrame->argument(0).toWTFString(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + + auto expected = callFrame->argument(1).toWTFString(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + + auto message = makeString("Protocol \""_s, actual, "\" not supported. Expected \""_s, expected, "\""_s); + return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_INVALID_PROTOCOL, message)); +} + } // namespace Bun JSC::JSValue WebCore::toJS(JSC::JSGlobalObject* globalObject, CommonAbortReason abortReason) diff --git a/src/bun.js/bindings/ErrorCode.h b/src/bun.js/bindings/ErrorCode.h index da3e5a01c788b..6d0828bea8e74 100644 --- a/src/bun.js/bindings/ErrorCode.h +++ b/src/bun.js/bindings/ErrorCode.h @@ -61,5 +61,6 @@ JSC_DECLARE_HOST_FUNCTION(jsFunction_ERR_IPC_DISCONNECTED); JSC_DECLARE_HOST_FUNCTION(jsFunction_ERR_SERVER_NOT_RUNNING); JSC_DECLARE_HOST_FUNCTION(jsFunction_ERR_IPC_CHANNEL_CLOSED); JSC_DECLARE_HOST_FUNCTION(jsFunction_ERR_SOCKET_BAD_TYPE); +JSC_DECLARE_HOST_FUNCTION(jsFunction_ERR_INVALID_PROTOCOL); } diff --git a/src/bun.js/bindings/ErrorCode.ts b/src/bun.js/bindings/ErrorCode.ts index 1b01afed6d90f..fdc964fb1c1a6 100644 --- a/src/bun.js/bindings/ErrorCode.ts +++ b/src/bun.js/bindings/ErrorCode.ts @@ -18,6 +18,7 @@ export default [ ["ERR_HTTP2_INVALID_SINGLE_VALUE_HEADER", TypeError, "TypeError"], ["ERR_INVALID_ARG_TYPE", TypeError, "TypeError"], ["ERR_INVALID_ARG_VALUE", TypeError, "TypeError"], + ["ERR_INVALID_PROTOCOL", TypeError, "TypeError"], ["ERR_INVALID_THIS", TypeError, "TypeError"], ["ERR_IPC_CHANNEL_CLOSED", Error, "Error"], ["ERR_IPC_DISCONNECTED", Error, "Error"], @@ -33,10 +34,11 @@ export default [ ["ERR_STREAM_DESTROYED", TypeError, "TypeError"], ["ERR_STREAM_NULL_VALUES", TypeError, "TypeError"], ["ERR_STREAM_WRITE_AFTER_END", TypeError, "TypeError"], - ["ERR_ZLIB_INITIALIZATION_FAILED", TypeError, "TypeError"], ["ERR_STRING_TOO_LONG", Error, "Error"], + ["ERR_ZLIB_INITIALIZATION_FAILED", TypeError, "TypeError"], ["ERR_ILLEGAL_CONSTRUCTOR", TypeError, "TypeError"], ["ERR_INVALID_URL", TypeError, "TypeError"], + // Bun-specific ["ERR_FORMDATA_PARSE_ERROR", TypeError, "TypeError"], ["ERR_BODY_ALREADY_USED", Error, "Error"], diff --git a/src/js/internal/cluster/Worker.ts b/src/js/internal/cluster/Worker.ts index b1940f151e062..14ff825c4dbd2 100644 --- a/src/js/internal/cluster/Worker.ts +++ b/src/js/internal/cluster/Worker.ts @@ -23,8 +23,7 @@ function Worker(options) { this.process.on("message", (message, handle) => this.emit("message", message, handle)); } } -Worker.prototype = {}; -ObjectSetPrototypeOf(Worker.prototype, EventEmitter.prototype); +Worker.prototype = Object.create(EventEmitter.prototype); Worker.prototype.kill = function () { this.destroy.$apply(this, arguments); diff --git a/src/js/internal/errors.ts b/src/js/internal/errors.ts index 4281a68059dc3..dedce8f38ad62 100644 --- a/src/js/internal/errors.ts +++ b/src/js/internal/errors.ts @@ -5,4 +5,5 @@ export default { ERR_SERVER_NOT_RUNNING: $newCppFunction("ErrorCode.cpp", "jsFunction_ERR_SERVER_NOT_RUNNING", 0), ERR_IPC_CHANNEL_CLOSED: $newCppFunction("ErrorCode.cpp", "jsFunction_ERR_IPC_CHANNEL_CLOSED", 0), ERR_SOCKET_BAD_TYPE: $newCppFunction("ErrorCode.cpp", "jsFunction_ERR_SOCKET_BAD_TYPE", 0), + ERR_INVALID_PROTOCOL: $newCppFunction("ErrorCode.cpp", "jsFunction_ERR_INVALID_PROTOCOL", 0), }; diff --git a/src/js/internal/url.ts b/src/js/internal/url.ts new file mode 100644 index 0000000000000..4349f82b6025c --- /dev/null +++ b/src/js/internal/url.ts @@ -0,0 +1,23 @@ +function urlToHttpOptions(url) { + const options = { + protocol: url.protocol, + hostname: + typeof url.hostname === "string" && url.hostname.startsWith("[") ? url.hostname.slice(1, -1) : url.hostname, + hash: url.hash, + search: url.search, + pathname: url.pathname, + path: `${url.pathname || ""}${url.search || ""}`, + href: url.href, + }; + if (url.port !== "") { + options.port = Number(url.port); + } + if (url.username || url.password) { + options.auth = `${decodeURIComponent(url.username)}:${decodeURIComponent(url.password)}`; + } + return options; +} + +export default { + urlToHttpOptions, +}; diff --git a/src/js/node/fs.ts b/src/js/node/fs.ts index 811b42c238b22..3b6f205101b4c 100644 --- a/src/js/node/fs.ts +++ b/src/js/node/fs.ts @@ -862,8 +862,7 @@ function ReadStream(this: typeof ReadStream, pathOrFd, options) { $assert(overridden_fs); this[kFs] = overridden_fs; } -ReadStream.prototype = {}; -ObjectSetPrototypeOf(ReadStream.prototype, NativeReadable.prototype); +ReadStream.prototype = Object.create(NativeReadable.prototype); ReadStream.prototype._construct = function (callback) { if (NativeReadablePrototype._construct) { diff --git a/src/js/node/http.ts b/src/js/node/http.ts index 1e879ea355f80..dcce2a1f56432 100644 --- a/src/js/node/http.ts +++ b/src/js/node/http.ts @@ -2,9 +2,10 @@ const EventEmitter = require("node:events"); const { isTypedArray } = require("node:util/types"); const { Duplex, Readable, Writable } = require("node:stream"); -const { ERR_INVALID_ARG_TYPE } = require("internal/errors"); +const { ERR_INVALID_ARG_TYPE, ERR_INVALID_PROTOCOL } = require("internal/errors"); const { isPrimary } = require("internal/cluster/isPrimary"); const { kAutoDestroyed } = require("internal/shared"); +const { urlToHttpOptions } = require("internal/url"); const { getHeader, @@ -97,7 +98,6 @@ const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; const NODE_HTTP_WARNING = "WARN: Agent is mostly unused in Bun's implementation of http. If you see strange behavior, this is probably the cause."; -var _defaultHTTPSAgent; var kInternalRequest = Symbol("kInternalRequest"); const kInternalSocketData = Symbol.for("::bunternal::"); const serverSymbol = Symbol.for("::bunternal::"); @@ -273,8 +273,7 @@ function Agent(options = kEmptyObject) { this.defaultPort = options.defaultPort || 80; this.protocol = options.protocol || "http:"; } -Agent.prototype = {}; -$setPrototypeDirect.$call(Agent.prototype, EventEmitter.prototype); +Agent.prototype = Object.create(EventEmitter.prototype); ObjectDefineProperty(Agent, "globalAgent", { get: function () { @@ -747,10 +746,6 @@ function assignHeaders(object, req) { var defaultIncomingOpts = { type: "request" }; -function getDefaultHTTPSAgent() { - return (_defaultHTTPSAgent ??= new Agent({ defaultPort: 443, protocol: "https:" })); -} - function requestHasNoBody(method, req) { if ("GET" === method || "HEAD" === method || "TRACE" === method || "CONNECT" === method || "OPTIONS" === method) return true; @@ -1654,38 +1649,26 @@ class ClientRequest extends OutgoingMessage { options = ObjectAssign(input || {}, options); } - var defaultAgent = options._defaultAgent || Agent.globalAgent; - - let protocol = options.protocol; - if (!protocol) { - if (options.port === 443) { - protocol = "https:"; - } else { - protocol = defaultAgent.protocol || "http:"; - } + let agent = options.agent; + const defaultAgent = options._defaultAgent || Agent.globalAgent; + if (agent === false) { + agent = new defaultAgent.constructor(); + } else if (agent == null) { + agent = defaultAgent; + } else if (typeof agent.addRequest !== "function") { + throw ERR_INVALID_ARG_TYPE("options.agent", "Agent-like Object, undefined, or false", agent); } - this.#protocol = protocol; + this.#agent = agent; - switch (this.#agent?.protocol) { - case undefined: { - break; - } - case "http:": { - if (protocol === "https:") { - defaultAgent = this.#agent = getDefaultHTTPSAgent(); - break; - } - } - case "https:": { - if (protocol === "https") { - defaultAgent = this.#agent = Agent.globalAgent; - break; - } - } - default: { - break; - } + const protocol = options.protocol || defaultAgent.protocol; + let expectedProtocol = defaultAgent.protocol; + if (this.agent.protocol) { + expectedProtocol = this.agent.protocol; } + if (protocol !== expectedProtocol) { + throw ERR_INVALID_PROTOCOL(protocol, expectedProtocol); + } + this.#protocol = protocol; if (options.path) { const path = String(options.path); @@ -1696,16 +1679,8 @@ class ClientRequest extends OutgoingMessage { } } - // Since we don't implement Agent, we don't need this - if (protocol !== "http:" && protocol !== "https:" && protocol) { - const expectedProtocol = defaultAgent?.protocol ?? "http:"; - throw new Error(`Protocol mismatch. Expected: ${expectedProtocol}. Got: ${protocol}`); - // throw new ERR_INVALID_PROTOCOL(protocol, expectedProtocol); - } - - const defaultPort = protocol === "https:" ? 443 : 80; - - this.#port = options.port || options.defaultPort || this.#agent?.defaultPort || defaultPort; + const defaultPort = options.defaultPort || this.#agent.defaultPort; + this.#port = options.port || defaultPort || 80; this.#useDefaultPort = this.#port === defaultPort; const host = (this.#host = @@ -1952,24 +1927,6 @@ class ClientRequest extends OutgoingMessage { } } -function urlToHttpOptions(url) { - var { protocol, hostname, hash, search, pathname, href, port, username, password } = url; - return { - protocol, - hostname: - typeof hostname === "string" && StringPrototypeStartsWith.$call(hostname, "[") - ? StringPrototypeSlice.$call(hostname, 1, -1) - : hostname, - hash, - search, - pathname, - path: `${pathname || ""}${search || ""}`, - href, - port: port ? Number(port) : protocol === "https:" ? 443 : protocol === "http:" ? 80 : undefined, - auth: username || password ? `${decodeURIComponent(username)}:${decodeURIComponent(password)}` : undefined, - }; -} - function validateHost(host, name) { if (host !== null && host !== undefined && typeof host !== "string") { // throw ERR_INVALID_ARG_TYPE( diff --git a/src/js/node/https.ts b/src/js/node/https.ts index d75fb3132ea7b..9f6c66abbdde5 100644 --- a/src/js/node/https.ts +++ b/src/js/node/https.ts @@ -1,14 +1,30 @@ // Hardcoded module "node:https" const http = require("node:http"); +const { urlToHttpOptions } = require("internal/url"); -function request(input, options, cb) { - if (input && typeof input === "object" && !(input instanceof URL)) { - input.protocol ??= "https:"; - } else if (typeof options === "object") { - options.protocol ??= "https:"; +const ObjectSetPrototypeOf = Object.setPrototypeOf; +const ArrayPrototypeShift = Array.prototype.shift; +const ObjectAssign = Object.assign; +const ArrayPrototypeUnshift = Array.prototype.unshift; + +function request(...args) { + let options = {}; + + if (typeof args[0] === "string") { + const urlStr = ArrayPrototypeShift.$call(args); + options = urlToHttpOptions(new URL(urlStr)); + } else if (args[0] instanceof URL) { + options = urlToHttpOptions(ArrayPrototypeShift.$call(args)); + } + + if (args[0] && typeof args[0] !== "function") { + ObjectAssign.$call(null, options, ArrayPrototypeShift.$call(args)); } - return http.request(input, options, cb); + options._defaultAgent = https.globalAgent; + ArrayPrototypeUnshift.$call(args, options); + + return new http.ClientRequest(...args); } function get(input, options, cb) { @@ -17,8 +33,24 @@ function get(input, options, cb) { return req; } -export default { - ...http, +function Agent(options) { + if (!(this instanceof Agent)) return new Agent(options); + + http.Agent.$apply(this, [options]); + this.defaultPort = 443; + this.protocol = "https:"; + this.maxCachedSessions = this.options.maxCachedSessions; + if (this.maxCachedSessions === undefined) this.maxCachedSessions = 100; +} +Agent.prototype = Object.create(http.Agent.prototype); +Agent.prototype.createConnection = http.createConnection; + +var https = { + Agent, + globalAgent: new Agent({ keepAlive: true, scheduling: "lifo", timeout: 5000 }), + Server: http.Server, + createServer: http.createServer, get, request, }; +export default https; diff --git a/src/js/node/url.ts b/src/js/node/url.ts index a8dd68a8e537a..d969ab08ff27c 100644 --- a/src/js/node/url.ts +++ b/src/js/node/url.ts @@ -25,6 +25,7 @@ const { URL, URLSearchParams } = globalThis; const [domainToASCII, domainToUnicode] = $cpp("NodeURL.cpp", "Bun::createNodeURLBinding"); +const { urlToHttpOptions } = require("internal/url"); function Url() { this.protocol = null; @@ -803,25 +804,6 @@ Url.prototype.parseHost = function () { this.hostname = host; } }; -function urlToHttpOptions(url) { - const options = { - protocol: url.protocol, - hostname: - typeof url.hostname === "string" && url.hostname.startsWith("[") ? url.hostname.slice(1, -1) : url.hostname, - hash: url.hash, - search: url.search, - pathname: url.pathname, - path: `${url.pathname || ""}${url.search || ""}`, - href: url.href, - }; - if (url.port !== "") { - options.port = Number(url.port); - } - if (url.username || url.password) { - options.auth = `${decodeURIComponent(url.username)}:${decodeURIComponent(url.password)}`; - } - return options; -} export default { parse: urlParse, diff --git a/src/js/node/zlib.ts b/src/js/node/zlib.ts index 16ac339edc521..c3e616b3c2ce8 100644 --- a/src/js/node/zlib.ts +++ b/src/js/node/zlib.ts @@ -48,8 +48,7 @@ function BrotliCompress(opts) { this[kHandle] = createBrotliEncoder(opts, {}, null); stream.Transform.$apply(this, arguments); } -BrotliCompress.prototype = {}; -ObjectSetPrototypeOf(BrotliCompress.prototype, stream.Transform.prototype); +BrotliCompress.prototype = Object.create(stream.Transform.prototype); BrotliCompress.prototype._transform = function _transform(chunk, encoding, callback) { callback(undefined, this[kHandle].encodeSync(chunk, encoding, false)); @@ -67,8 +66,7 @@ function BrotliDecompress(opts) { this[kHandle] = createBrotliDecoder(opts, {}, null); stream.Transform.$apply(this, arguments); } -BrotliDecompress.prototype = {}; -ObjectSetPrototypeOf(BrotliDecompress.prototype, stream.Transform.prototype); +BrotliDecompress.prototype = Object.create(stream.Transform.prototype); BrotliDecompress.prototype._transform = function (chunk, encoding, callback) { callback(undefined, this[kHandle].decodeSync(chunk, encoding, false)); diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index 8dd692e38e960..a2022c818079b 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -429,7 +429,7 @@ describe("node:http", () => { }); it("should make a https:// GET request when passed string as first arg", done => { - const req = request("https://example.com", { headers: { "accept-encoding": "identity" } }, res => { + const req = https.request("https://example.com", { headers: { "accept-encoding": "identity" } }, res => { let data = ""; res.setEncoding("utf8"); res.on("data", chunk => { @@ -1090,12 +1090,14 @@ describe("node:http", () => { test("should not decompress gzip, issue#4397", async () => { const { promise, resolve } = Promise.withResolvers(); - request("https://bun.sh/", { headers: { "accept-encoding": "gzip" } }, res => { - res.on("data", function cb(chunk) { - resolve(chunk); - res.off("data", cb); - }); - }).end(); + https + .request("https://bun.sh/", { headers: { "accept-encoding": "gzip" } }, res => { + res.on("data", function cb(chunk) { + resolve(chunk); + res.off("data", cb); + }); + }) + .end(); const chunk = await promise; expect(chunk.toString()).not.toContain(" { expect(stdout.toString()).toContain("Test passed"); expect(exitCode).toBe(0); }); + // This test is disabled because it can OOM the CI it.skip("should be able to stream huge amounts of data", async () => { const buf = Buffer.alloc(1024 * 1024 * 256); @@ -2265,7 +2268,7 @@ it.skip("should be able to stream huge amounts of data", async () => { // TODO: today we use a workaround to continue event, we need to fix it in the future. it("should emit continue event #7480", done => { let receivedContinue = false; - const req = request( + const req = https.request( "https://example.com", { headers: { "accept-encoding": "identity", "expect": "100-continue" } }, res => { @@ -2290,7 +2293,7 @@ it("should emit continue event #7480", done => { it("should not emit continue event #7480", done => { let receivedContinue = false; - const req = request("https://example.com", { headers: { "accept-encoding": "identity" } }, res => { + const req = https.request("https://example.com", { headers: { "accept-encoding": "identity" } }, res => { let data = ""; res.setEncoding("utf8"); res.on("data", chunk => { @@ -2308,3 +2311,57 @@ it("should not emit continue event #7480", done => { }); req.end(); }); + +it("http.Agent is configured correctly", () => { + const agent = new http.Agent(); + expect(agent.defaultPort).toBe(80); + expect(agent.protocol).toBe("http:"); +}); + +it("https.Agent is configured correctly", () => { + const agent = new https.Agent(); + expect(agent.defaultPort).toBe(443); + expect(agent.protocol).toBe("https:"); +}); + +it("http.get can use http.Agent", async () => { + const agent = new http.Agent(); + const { promise, resolve } = Promise.withResolvers(); + http.get({ agent, hostname: "google.com" }, resolve); + const response = await promise; + expect(response.req.port).toBe(80); + expect(response.req.protocol).toBe("http:"); +}); + +it("https.get can use https.Agent", async () => { + const agent = new https.Agent(); + const { promise, resolve } = Promise.withResolvers(); + https.get({ agent, hostname: "google.com" }, resolve); + const response = await promise; + expect(response.req.port).toBe(443); + expect(response.req.protocol).toBe("https:"); +}); + +it("http.request has the correct options", async () => { + const { promise, resolve } = Promise.withResolvers(); + http.request("http://google.com/", resolve).end(); + const response = await promise; + expect(response.req.port).toBe(80); + expect(response.req.protocol).toBe("http:"); +}); + +it("https.request has the correct options", async () => { + const { promise, resolve } = Promise.withResolvers(); + https.request("https://google.com/", resolve).end(); + const response = await promise; + expect(response.req.port).toBe(443); + expect(response.req.protocol).toBe("https:"); +}); + +it("using node:http to do https: request fails", () => { + expect(() => http.request("https://example.com")).toThrow(TypeError); + expect(() => http.request("https://example.com")).toThrow({ + code: "ERR_INVALID_PROTOCOL", + message: `Protocol "https:" not supported. Expected "http:"`, + }); +});