Skip to content

Commit 2238c75

Browse files
satyarohithzebreus
authored andcommitted
fix(ext/node): don't wait for end() call to send http client request (denoland#24390)
Closes denoland#24232 Closes denoland#24215
1 parent 8af1a70 commit 2238c75

File tree

3 files changed

+131
-49
lines changed

3 files changed

+131
-49
lines changed

ext/node/polyfills/http.ts

+58-49
Original file line numberDiff line numberDiff line change
@@ -623,62 +623,13 @@ class ClientRequest extends OutgoingMessage {
623623
client[internalRidSymbol],
624624
this._bodyWriteRid,
625625
);
626-
}
627-
628-
_implicitHeader() {
629-
if (this._header) {
630-
throw new ERR_HTTP_HEADERS_SENT("render");
631-
}
632-
this._storeHeader(
633-
this.method + " " + this.path + " HTTP/1.1\r\n",
634-
this[kOutHeaders],
635-
);
636-
}
637-
638-
_getClient(): Deno.HttpClient | undefined {
639-
return undefined;
640-
}
641-
642-
// TODO(bartlomieju): handle error
643-
onSocket(socket, _err) {
644-
nextTick(() => {
645-
this.socket = socket;
646-
this.emit("socket", socket);
647-
});
648-
}
649-
650-
// deno-lint-ignore no-explicit-any
651-
end(chunk?: any, encoding?: any, cb?: any): this {
652-
if (typeof chunk === "function") {
653-
cb = chunk;
654-
chunk = null;
655-
encoding = null;
656-
} else if (typeof encoding === "function") {
657-
cb = encoding;
658-
encoding = null;
659-
}
660-
661-
this.finished = true;
662-
if (chunk) {
663-
this.write_(chunk, encoding, null, true);
664-
} else if (!this._headerSent) {
665-
this._contentLength = 0;
666-
this._implicitHeader();
667-
this._send("", "latin1");
668-
}
669-
this._bodyWriter?.close();
670626

671627
(async () => {
672628
try {
673629
const res = await op_fetch_send(this._req.requestRid);
674630
if (this._req.cancelHandleRid !== null) {
675631
core.tryClose(this._req.cancelHandleRid);
676632
}
677-
try {
678-
cb?.();
679-
} catch (_) {
680-
//
681-
}
682633
if (this._timeout) {
683634
this._timeout.removeEventListener("abort", this._timeoutCb);
684635
webClearTimeout(this._timeout[timerId]);
@@ -788,6 +739,64 @@ class ClientRequest extends OutgoingMessage {
788739
})();
789740
}
790741

742+
_implicitHeader() {
743+
if (this._header) {
744+
throw new ERR_HTTP_HEADERS_SENT("render");
745+
}
746+
this._storeHeader(
747+
this.method + " " + this.path + " HTTP/1.1\r\n",
748+
this[kOutHeaders],
749+
);
750+
}
751+
752+
_getClient(): Deno.HttpClient | undefined {
753+
return undefined;
754+
}
755+
756+
// TODO(bartlomieju): handle error
757+
onSocket(socket, _err) {
758+
nextTick(() => {
759+
this.socket = socket;
760+
this.emit("socket", socket);
761+
});
762+
}
763+
764+
// deno-lint-ignore no-explicit-any
765+
end(chunk?: any, encoding?: any, cb?: any): this {
766+
if (typeof chunk === "function") {
767+
cb = chunk;
768+
chunk = null;
769+
encoding = null;
770+
} else if (typeof encoding === "function") {
771+
cb = encoding;
772+
encoding = null;
773+
}
774+
775+
this.finished = true;
776+
if (chunk) {
777+
this.write_(chunk, encoding, null, true);
778+
} else if (!this._headerSent) {
779+
this._contentLength = 0;
780+
this._implicitHeader();
781+
this._send("", "latin1");
782+
}
783+
(async () => {
784+
try {
785+
await this._bodyWriter?.close();
786+
} catch (_) {
787+
// The readable stream resource is dropped right after
788+
// read is complete closing the writable stream resource.
789+
// If we try to close the writer again, it will result in an
790+
// error which we can safely ignore.
791+
}
792+
try {
793+
cb?.();
794+
} catch (_) {
795+
//
796+
}
797+
})();
798+
}
799+
791800
abort() {
792801
if (this.aborted) {
793802
return;

tests/unit_node/http_test.ts

+72
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@ import http, { type RequestOptions } from "node:http";
55
import url from "node:url";
66
import https from "node:https";
77
import net from "node:net";
8+
import fs from "node:fs";
9+
810
import { assert, assertEquals, fail } from "@std/assert/mod.ts";
911
import { assertSpyCalls, spy } from "@std/testing/mock.ts";
12+
import { fromFileUrl, relative } from "@std/path/mod.ts";
1013

1114
import { gzip } from "node:zlib";
1215
import { Buffer } from "node:buffer";
@@ -1179,3 +1182,72 @@ Deno.test("[node/http] client closing a streaming response doesn't terminate ser
11791182
assertEquals(server.listening, false);
11801183
clearInterval(interval!);
11811184
});
1185+
1186+
Deno.test("[node/http] http.request() post streaming body works", async () => {
1187+
const server = http.createServer((req, res) => {
1188+
if (req.method === "POST") {
1189+
let receivedBytes = 0;
1190+
req.on("data", (chunk) => {
1191+
receivedBytes += chunk.length;
1192+
});
1193+
req.on("end", () => {
1194+
res.writeHead(200, { "Content-Type": "application/json" });
1195+
res.end(JSON.stringify({ bytes: receivedBytes }));
1196+
});
1197+
} else {
1198+
res.writeHead(405, { "Content-Type": "text/plain" });
1199+
res.end("Method Not Allowed");
1200+
}
1201+
});
1202+
1203+
const deferred = Promise.withResolvers<void>();
1204+
const timeout = setTimeout(() => {
1205+
deferred.reject(new Error("timeout"));
1206+
}, 5000);
1207+
server.listen(0, () => {
1208+
// deno-lint-ignore no-explicit-any
1209+
const port = (server.address() as any).port;
1210+
const filePath = relative(
1211+
Deno.cwd(),
1212+
fromFileUrl(new URL("./testdata/lorem_ipsum_512kb.txt", import.meta.url)),
1213+
);
1214+
const contentLength = 524289;
1215+
1216+
const options = {
1217+
hostname: "localhost",
1218+
port: port,
1219+
path: "/",
1220+
method: "POST",
1221+
headers: {
1222+
"Content-Type": "application/octet-stream",
1223+
"Content-Length": contentLength,
1224+
},
1225+
};
1226+
1227+
const req = http.request(options, (res) => {
1228+
let responseBody = "";
1229+
res.on("data", (chunk) => {
1230+
responseBody += chunk;
1231+
});
1232+
1233+
res.on("end", () => {
1234+
const response = JSON.parse(responseBody);
1235+
assertEquals(res.statusCode, 200);
1236+
assertEquals(response.bytes, contentLength);
1237+
deferred.resolve();
1238+
});
1239+
});
1240+
1241+
req.on("error", (e) => {
1242+
console.error(`Problem with request: ${e.message}`);
1243+
});
1244+
1245+
const readStream = fs.createReadStream(filePath);
1246+
readStream.pipe(req);
1247+
});
1248+
await deferred.promise;
1249+
assertEquals(server.listening, true);
1250+
server.close();
1251+
clearTimeout(timeout);
1252+
assertEquals(server.listening, false);
1253+
});

tests/unit_node/testdata/lorem_ipsum_512kb.txt

+1
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)