nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.27k stars 324 forks source link

Missing http.ServerResponse.flushHeaders function #1006

Closed alexluke closed 7 months ago

alexluke commented 7 months ago

We have a nextjs 14.0.2 app that is built in standalone mode. We're using nginx unit for TLS termination.

When responding to a request, the following error is thrown.

2023/11/15 13:58:32 [info] 70#70 discovery started
2023/11/15 13:58:32 [notice] 70#70 no modules matching: "/usr/lib/unit/modules/*.unit.so" found
2023/11/15 13:58:32 [info] 1#1 controller started
2023/11/15 13:58:32 [notice] 1#1 process 70 exited with code 0
2023/11/15 13:58:32 [info] 72#72 router started
2023/11/15 13:58:32 [info] 72#72 OpenSSL 1.1.1w  11 Sep 2023, 1010117f
2023/11/15 13:58:32 [info] 73#73 "nextjs" prototype started
2023/11/15 13:58:32 [info] 74#74 "nextjs" application started
(node:74) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("unit-http/loader.mjs", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
   ▲ Next.js 14.0.2
   - Local:        http://95e6f3d5ed03:80
   - Network:      http://127.0.0.1:80

 ✓ Ready in 40ms
 ⨯ Error: failed to pipe response
    at ei (/app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:210527)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:540:9)
    at process.processTimers (node:internal/timers:514:7)
    at async es.pipeToNodeResponse (/app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:212125)
    at async sendRenderResult (/app/node_modules/next/dist/server/send-payload.js:83:5)
    at async NextNodeServer.pipeImpl (/app/node_modules/next/dist/server/base-server.js:917:13)
    at async NextNodeServer.handleCatchallRenderRequest (/app/node_modules/next/dist/server/next-server.js:266:17)
    at async NextNodeServer.handleRequestImpl (/app/node_modules/next/dist/server/base-server.js:798:17) {
  [cause]: TypeError: e.flushHeaders is not a function
      at Object.write (/app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:210155)
      at ensureIsPromise (node:internal/webstreams/util:185:19)
      at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1109:5)
      at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1224:5)
      at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1098:3)
      at writableStreamDefaultWriterWrite (node:internal/webstreams/writablestream:988:3)
      at [kChunk] (node:internal/webstreams/readablestream:1545:31)
      at readableStreamFulfillReadRequest (node:internal/webstreams/readablestream:2087:24)
      at readableStreamDefaultControllerEnqueue (node:internal/webstreams/readablestream:2278:5)
      at transformStreamDefaultControllerEnqueue (node:internal/webstreams/transformstream:486:5)

This appears to be because the flushHeaders method doesn't exist on the ServerResponse under nginx unit. The app runs fine when using node's own http module.

https://github.com/vercel/next.js/blob/v14.0.2/packages/next/src/server/pipe-readable.ts#L48

https://nodejs.org/api/http.html#responseflushheaders

Please let me know what other detail would be helpful.

andrey-zelenkov commented 7 months ago

Hi @alexluke, thank you for your report.

Could you please try following patch if it works for you:

--- a/src/nodejs/unit-http/http_server.js
+++ b/src/nodejs/unit-http/http_server.js
@@ -160,6 +160,10 @@ ServerResponse.prototype._removeHeader =
     this.headers_len -= name_len + Buffer.byteLength(value + "", 'latin1');
 };

+ServerResponse.prototype.flushHeaders = function flushHeaders() {
+    this._sendHeaders();
+}
+
 ServerResponse.prototype.sendDate = function sendDate() {
     throw new Error("Not supported");
 };
ac000 commented 7 months ago

--- a/src/nodejs/unit-http/http_server.js
+++ b/src/nodejs/unit-http/http_server.js
@@ -160,6 +160,10 @@ ServerResponse.prototype._removeHeader =
     this.headers_len -= name_len + Buffer.byteLength(value + "", 'latin1');
 };

+ServerResponse.prototype.flushHeaders = function flushHeaders() {
+    this._sendHeaders();
+}

Any reason there's no trailing ; here?

andrey-zelenkov commented 7 months ago

No, it was just my inattention. Thank you for noticing! Additionally, I've moved this code slightly higher to reduce the confusion between private and public methods ordering for headers:

diff --git a/src/nodejs/unit-http/http_server.js b/src/nodejs/unit-http/http_server.js
--- a/src/nodejs/unit-http/http_server.js
+++ b/src/nodejs/unit-http/http_server.js
@@ -138,6 +138,10 @@ ServerResponse.prototype.removeHeader = 
     }
 };

+ServerResponse.prototype.flushHeaders = function flushHeaders() {
+    this._sendHeaders();
+};
+
 ServerResponse.prototype._removeHeader = function _removeHeader(lc_name) {
     let entry = this.headers[lc_name];
     let name_len = Buffer.byteLength(entry[0] + "", 'latin1');
ac000 commented 7 months ago

I can't honestly say whether this is correct or not, but it looks simple enough, so assuming it works... consider it...

Reviewed-by: Andrew Clayton <a.clayton@nginx.com>