Skip to content

reduce buffer copies #867

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.

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

Merged
merged 1 commit into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/strange-cars-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@smithy/util-body-length-node": patch
"@smithy/node-http-handler": patch
---

reduce buffer copies
61 changes: 33 additions & 28 deletions packages/node-http-handler/src/node-http-handler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import { NodeHttpHandler } from "./node-http-handler";
import { ReadFromBuffers } from "./readable.mock";
import {
createContinueResponseFunction,
createMirrorResponseFunction,
createMockHttpServer,
createMockHttpsServer,
createResponseFunction,
getResponseBody,
} from "./server.mock";

describe("NodeHttpHandler", () => {
Expand Down Expand Up @@ -203,36 +205,39 @@ describe("NodeHttpHandler", () => {
expect(response.body).toBeDefined();
});

it("can send requests with bodies", async () => {
const body = Buffer.from("test");
const mockResponse = {
statusCode: 200,
headers: {},
};
mockHttpServer.addListener("request", createResponseFunction(mockResponse));
const spy = jest.spyOn(http, "request").mockImplementationOnce(() => {
const calls = spy.mock.calls;
const currentIndex = calls.length - 1;
return http.request(calls[currentIndex][0], calls[currentIndex][1]);
});

const nodeHttpHandler = new NodeHttpHandler();
const { response } = await nodeHttpHandler.handle(
new HttpRequest({
hostname: "localhost",
method: "PUT",
port: (mockHttpServer.address() as AddressInfo).port,
protocol: "http:",
path: "/",
[
{ name: "buffer", body: Buffer.from("Buffering🚀") },
{ name: "uint8Array", body: Uint8Array.from(Buffer.from("uint8Array 🚀")) },
{ name: "string", body: Buffer.from("string-test 🚀") },
{ name: "uint8Array subarray", body: Uint8Array.from(Buffer.from("test")).subarray(1, 3) },
{ name: "buffer subarray", body: Buffer.from("test").subarray(1, 3) },
].forEach(({ body, name }) => {
it(`can send requests with bodies ${name}`, async () => {
const mockResponse = {
statusCode: 200,
headers: {},
body,
}),
{}
);
};
mockHttpServer.addListener("request", createMirrorResponseFunction(mockResponse));
const nodeHttpHandler = new NodeHttpHandler();
const { response } = await nodeHttpHandler.handle(
new HttpRequest({
hostname: "localhost",
method: "PUT",
port: (mockHttpServer.address() as AddressInfo).port,
protocol: "http:",
path: "/",
headers: {},
body,
}),
{}
);

expect(response.statusCode).toEqual(mockResponse.statusCode);
expect(response.headers).toBeDefined();
expect(response.headers).toMatchObject(mockResponse.headers);
expect(response.statusCode).toEqual(mockResponse.statusCode);
expect(response.headers).toBeDefined();
expect(response.headers).toMatchObject(mockResponse.headers);
const responseBody = await getResponseBody(response);
expect(responseBody).toEqual(Buffer.from(body).toString());
});
});

it("can handle expect 100-continue", async () => {
Expand Down
39 changes: 37 additions & 2 deletions packages/node-http-handler/src/server.mock.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { HeaderBag, HttpResponse } from "@smithy/types";
import { HeaderBag, HttpResponse, NodeJsRuntimeBlobTypes } from "@smithy/types";
import { readFileSync } from "fs";
import { createServer as createHttpServer, IncomingMessage, Server as HttpServer, ServerResponse } from "http";
import { createServer as createHttp2Server, Http2Server } from "http2";
Expand All @@ -14,7 +14,7 @@ const setResponseHeaders = (response: ServerResponse, headers: HeaderBag) => {
}
};

const setResponseBody = (response: ServerResponse, body: Readable | string) => {
const setResponseBody = (response: ServerResponse, body: string | NodeJsRuntimeBlobTypes) => {
if (body instanceof Readable) {
body.pipe(response);
} else {
Expand Down Expand Up @@ -67,3 +67,38 @@ export const createMockHttp2Server = (): Http2Server => {
const server = createHttp2Server();
return server;
};

export const createMirrorResponseFunction = (httpResp: HttpResponse) => (
request: IncomingMessage,
response: ServerResponse
) => {
const bufs: Buffer[] = [];
request.on("data", (chunk) => {
bufs.push(chunk);
});
request.on("end", () => {
response.statusCode = httpResp.statusCode;
setResponseHeaders(response, httpResp.headers);
setResponseBody(response, Buffer.concat(bufs));
});
request.on("error", (err) => {
response.statusCode = 500;
setResponseHeaders(response, httpResp.headers);
setResponseBody(response, err.message);
});
};

export const getResponseBody = (response: HttpResponse) => {
return new Promise<string>((resolve, reject) => {
const bufs: Buffer[] = [];
response.body.on("data", function (d: Buffer) {
bufs.push(d);
});
response.body.on("end", function () {
resolve(Buffer.concat(bufs).toString());
});
response.body.on("error", (err: Error) => {
reject(err);
});
});
};
29 changes: 25 additions & 4 deletions packages/node-http-handler/src/write-request-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,30 @@ function writeBody(
if (body instanceof Readable) {
// pipe automatically handles end
body.pipe(httpRequest);
} else if (body) {
httpRequest.end(Buffer.from(body as Parameters<typeof Buffer.from>[0]));
} else {
httpRequest.end();
return;
}

if (body) {
if (Buffer.isBuffer(body) || typeof body === "string") {
httpRequest.end(body);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientRequest (node:http(s)) and ClientHttp2Stream (node:http2) both accept .end(string) in addition to .end(Buffer).

return;
}

const uint8 = body as Uint8Array;
if (
typeof uint8 === "object" &&
uint8.buffer &&
typeof uint8.byteOffset === "number" &&
typeof uint8.byteLength === "number"
) {
// this avoids copying the array.
httpRequest.end(Buffer.from(uint8.buffer, uint8.byteOffset, uint8.byteLength));
return;
}

httpRequest.end(Buffer.from(body as ArrayBuffer));
return;
}

httpRequest.end();
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const calculateBodyLength = (body: any): number | undefined => {
return 0;
}
if (typeof body === "string") {
return Buffer.from(body).length;
return Buffer.byteLength(body);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this works in Node.js 14+

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> Buffer.byteLength('🚀a')
5
> Buffer.from('🚀a').length
5

} else if (typeof body.byteLength === "number") {
// handles Uint8Array, ArrayBuffer, Buffer, and ArrayBufferView
return body.byteLength;
Expand Down