Skip to content

avoid unnecessary buffer copies on opennext.js response #1010

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

avoid unnecessary buffer copies on opennext.js response #1010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/nasty-houses-relate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@opennextjs/aws": patch
---

Cache opennext response body to avoid unnecessary copies
89 changes: 55 additions & 34 deletions packages/open-next/src/http/openNextResponse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ import { parseHeaders, parseSetCookieHeader } from "./util";

const SET_COOKIE_HEADER = "set-cookie";
const CANNOT_BE_USED = "This cannot be used in OpenNext";
const ERROR_CACHE_CONTROL_HEADER =
"private, no-cache, no-store, max-age=0, must-revalidate";

// Cache environment variable checks at module load time
const FORCE_NON_EMPTY_RESPONSE =
process.env.OPEN_NEXT_FORCE_NON_EMPTY_RESPONSE === "true";
const DANGEROUSLY_SET_ERROR_HEADERS =
process.env.OPEN_NEXT_DANGEROUSLY_SET_ERROR_HEADERS === "true";

// We only need to implement the methods that are used by next.js
export class OpenNextNodeResponse extends Transform implements ServerResponse {
Expand All @@ -26,6 +34,7 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
private _cookies: string[] = [];
private responseStream?: Writable;
private bodyLength = 0;
private _cachedBody?: Buffer;

// To comply with the ServerResponse interface :
strictContentLength = false;
Expand Down Expand Up @@ -103,7 +112,7 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {

get finished() {
return this.responseStream
? this.responseStream?.writableFinished
? this.responseStream.writableFinished
Copy link
Contributor

Choose a reason for hiding this comment

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

@anonrig ... just a general note. This looks like a drive-by change unrelated to the actual unnecessary buffer copies. For additional changes like this, can you put these into separate commits moving forward? Makes it a bit difficult to separate what is or is not relevant to the actual allocation/copy improvements.

: this.writableFinished;
}

Expand Down Expand Up @@ -172,13 +181,20 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
...this.initialHeaders,
...this.headers,
};
const initialCookies = parseSetCookieHeader(
this.initialHeaders[SET_COOKIE_HEADER]?.toString(),
);
this._cookies =
mergeHeadersPriority === "middleware"
? [...this._cookies, ...initialCookies]
: [...initialCookies, ...this._cookies];
// Only parse initial cookies if they exist
const initialCookieHeader = this.initialHeaders[SET_COOKIE_HEADER];
if (initialCookieHeader) {
const initialCookies = parseSetCookieHeader(
initialCookieHeader.toString(),
);
if (initialCookies.length > 0) {
if (mergeHeadersPriority === "middleware") {
this._cookies.push(...initialCookies);
} else {
this._cookies.unshift(...initialCookies);
}
}
}
}
this.fixHeaders(this.headers);
this.fixHeadersForError();
Expand All @@ -204,15 +220,29 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {

appendHeader(name: string, value: string | string[]): this {
const key = name.toLowerCase();
if (!this.hasHeader(key)) {
return this.setHeader(key, value);
// Special handling for set-cookie header
if (key === SET_COOKIE_HEADER) {
const toAppend = Array.isArray(value) ? value : [value];
this._cookies.push(...toAppend);
this.headers[key] = this._cookies;
return this;
}

const existingHeader = this.headers[key];
if (existingHeader === undefined) {
this.headers[key] = value;
return this;
}
const existingHeader = this.getHeader(key) as string | string[];

// Merge headers efficiently
const toAppend = Array.isArray(value) ? value : [value];
const newValue = Array.isArray(existingHeader)
? [...existingHeader, ...toAppend]
: [existingHeader, ...toAppend];
return this.setHeader(key, newValue);
if (Array.isArray(existingHeader)) {
existingHeader.push(...toAppend);
this.headers[key] = existingHeader;
} else {
this.headers[key] = [existingHeader as string, ...toAppend];
}
return this;
}

// Might be used in next page api routes
Expand Down Expand Up @@ -243,26 +273,21 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
| OutgoingHttpHeader[]
| undefined;
}
const finalHeaders: OutgoingHttpHeaders = this.headers;
if (_headers) {
if (Array.isArray(_headers)) {
// headers may be an Array where the keys and values are in the same list. It is not a list of tuples. So, the even-numbered offsets are key values, and the odd-numbered offsets are the associated values.
for (let i = 0; i < _headers.length; i += 2) {
finalHeaders[_headers[i] as string] = _headers[i + 1] as
this.headers[_headers[i] as string] = _headers[i + 1] as
| string
| string[];
}
} else {
for (const key of Object.keys(_headers)) {
finalHeaders[key] = _headers[key];
}
// Use Object.assign for better performance with plain objects
Object.assign(this.headers, _headers);
}
}

this.statusCode = statusCode as number;
if (headers) {
this.headers = finalHeaders;
}
this.flushHeaders();
return this;
}
Expand All @@ -281,7 +306,8 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
}

getBody() {
return Buffer.concat(this._chunks);
this._cachedBody ??= Buffer.concat(this._chunks);
return this._cachedBody;
}

private _internalWrite(chunk: any, encoding: BufferEncoding) {
Expand All @@ -290,6 +316,8 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
if (this.streamCreator?.retainChunks !== false) {
// Avoid keeping chunks around when the `StreamCreator` supports it to save memory
this._chunks.push(buffer);
// Invalidate cached body when new chunks are added
this._cachedBody = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a bit problematic for memory. This means that we're persistently doubling the amount of memory consumed. Sure it means we eliminate multiple potential copies but at the cost of definitely holding more memory.

Instead, if we're going to cache this, we should replace this._chunks with the combined buffer so that the separate chunks can be reclaimed.

I think a better approach is to see if we can eliminate this entirely, and instead rely on regular stream buffering.

Ultimately I'm not sure if this is the right change.

}
this.push(chunk, encoding);
this.streamCreator?.onWrite?.();
Expand Down Expand Up @@ -325,13 +353,7 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
//If not you can set the OPEN_NEXT_FORCE_NON_EMPTY_RESPONSE env variable to true
//BE CAREFUL: Aws keeps rolling out broken streaming implementations even on accounts that had working ones before
//This is not dependent on the node runtime used
if (
this.bodyLength === 0 &&
// We use an env variable here because not all aws account have the same behavior
// On some aws accounts the response will hang if the body is empty
// We are modifying the response body here, this is not a good practice
process.env.OPEN_NEXT_FORCE_NON_EMPTY_RESPONSE === "true"
) {
if (this.bodyLength === 0 && FORCE_NON_EMPTY_RESPONSE) {
debug('Force writing "SOMETHING" to the response body');
this.push("SOMETHING");
}
Expand Down Expand Up @@ -404,16 +426,15 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
// For some reason, next returns the 500 error page with some cache-control headers
// We need to fix that
private fixHeadersForError() {
if (process.env.OPEN_NEXT_DANGEROUSLY_SET_ERROR_HEADERS === "true") {
if (DANGEROUSLY_SET_ERROR_HEADERS) {
return;
}
// We only check for 404 and 500 errors
// The rest should be errors that are handled by the user and they should set the cache headers themselves
if (this.statusCode === 404 || this.statusCode === 500) {
// For some reason calling this.setHeader("Cache-Control", "no-cache, no-store, must-revalidate") does not work here
// The function is not even called, i'm probably missing something obvious
this.headers["cache-control"] =
"private, no-cache, no-store, max-age=0, must-revalidate";
this.headers["cache-control"] = ERROR_CACHE_CONTROL_HEADER;
}
}
}
Loading