fix: Take options as well as requestListener #1091

javorszky · 2024-01-25T15:26:00Z

Unit-http have not kept up with the signature of nodejs's http package development. Nodejs allows an optional options object to be passed to the createServer function, we didn't. This resulted in function signature errors when user code that did make use of the options arg tried to call unit's replaced function.

This change changes the signature to be more in line with how nodejs does it discarding it and printing a message to stderr so it shows up in unit logs.

javorszky · 2024-01-25T15:28:13Z

I tested with the following nodejs application file at /Users/g.javorszky/Projects/Unit/issue-1043/flob/index.js:

const http = require("http");

const host = 'localhost';
const port = 8000;

const requestListener = function (req, res) {
    res.writeHead(200);
    res.end("My first server!");
};

const options = {
    keepAlive: true
};

const server = http.createServer(options, requestListener);
server.listen(port, host, () => {
    console.log(`Server is running on http://${host}:${port}`);
});

and the following unit configuration file:

{
    "listeners": {
        "*:5555": {
            "pass": "routes/main"
        }
    },
    "routes": {
        "main": [
            {
                "action": {
                    "pass": "applications/app"
                }
            }
        ]
    },
    "applications": {
        "app": {
            "type": "external",
            "working_directory": "/Users/g.javorszky/Projects/Unit/issue-1043/flob/",
            "executable": "/usr/bin/env",
            "arguments": [
                "/Users/g.javorszky/.nvm/versions/node/v20.9.0/bin/node",
                "--loader",
                "/Users/g.javorszky/.nvm/versions/node/v20.9.0/lib/node_modules/unit-http/loader.mjs",
                "--require",
                "/Users/g.javorszky/.nvm/versions/node/v20.9.0/lib/node_modules/unit-http/loader",
                "index.js"
            ]
        }
    }
}

I had to use absolute paths for the node executable and the two loader files.

Given these, navigating to http://localhost:5555 gives me a 200 OK with text "My first server" as expected.

src/nodejs/unit-http/http.js

javorszky · 2024-01-26T16:28:55Z

@andrey-zelenkov I added the docs to changes in cdf9bfe as a response to your comment on the issue here: #1013 (comment)

Everyone, let me know if the wording is appropriate, or I should tweak it.

src/nodejs/unit-http/http.js

test/node/options/app.js

docs/changes.xml

src/nodejs/unit-http/http_server.js

ac000 · 2024-01-30T01:48:53Z

For

 b090121 Take options as well as requestListener

I would rather the Closes:, with a :, was at the bottom the commit message, separated by a blank line, for consistency. But other than that.

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

Only want to approve the first patch not the whole thing.

test/test_node_application.py

src/nodejs/unit-http/http_server.js

javorszky · 2024-02-08T13:47:43Z

@andrey-zelenkov I added the test, and in that I'm checking that the log DOES have the log line added, and the test passes for me locally. I have not changed any of the other tests.

andrey-zelenkov · 2024-02-08T17:13:25Z

Currently this patch breaks our nodejs module on RHEL8:

% uname -a
Linux ip-10-1-18-198.eu-central-1.compute.internal 4.18.0-477.27.1.el8_8.x86_64 #1 SMP Thu Aug 31 10:29:22 EDT 2023 x86_64 x86_64 x86_64 GNU/Linux

% cat /etc/os-release
NAME="Red Hat Enterprise Linux"
VERSION="8.8 (Ootpa)"
ID="rhel"
ID_LIKE="fedora"
VERSION_ID="8.8"
PLATFORM_ID="platform:el8"
PRETTY_NAME="Red Hat Enterprise Linux 8.8 (Ootpa)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:redhat:enterprise_linux:8::baseos"
HOME_URL="https://www.redhat.com/"
DOCUMENTATION_URL="https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8"
BUG_REPORT_URL="https://bugzilla.redhat.com/"

REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 8"
REDHAT_BUGZILLA_PRODUCT_VERSION=8.8
REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux"
REDHAT_SUPPORT_PRODUCT_VERSION="8.8"

If you send request to nodejs app it will fail with 503:

>>>
GET / HTTP/1.1
Host: localhost
Connection: close


<<<
HTTP/1.1 503 Service Unavailable
Content-Type: text/html
Server: Unit/1.32.0
Date: Thu, 08 Feb 2024 16:53:49 GMT
Content-Length: 54
Connection: close

<!DOCTYPE html><title>Error 503</title><p>Error 503.

Error message from unit.log:

    throw err;
    ^

Error: Cannot find module 'node:process'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/tmp/unit-test-2wn2yn68/node/node_modules/unit-http/http_server.js:8:20)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)

From my side, I can suggest just dropping error reporting (that uses node:process) for now and committing it later once RHEL8 is outdated. It will allow us to fix issues without breaking any existing builds at the same time.

andrey-zelenkov · 2024-02-08T17:46:10Z

fyi, applying this diff fixes the problem:

--- a/src/nodejs/unit-http/http_server.js
+++ b/src/nodejs/unit-http/http_server.js
@@ -5,7 +5,6 @@
 
 'use strict';
 
-const { stderr } = require('node:process');
 const EventEmitter = require('events');
 const http = require('http');
 const util = require('util');
@@ -418,8 +417,6 @@ function Server(options, requestListener
     if (typeof options === 'function') {
         requestListener = options;
         options = {};
-    } else {
-        stderr.write("http.Server constructor was called with unsupported options, using default settings\n");
     }
 
     EventEmitter.call(this);
diff --git a/test/test_node_application.py b/test/test_node_application.py
--- a/test/test_node_application.py
+++ b/test/test_node_application.py
@@ -21,11 +21,10 @@ def test_node_application_basic():
 
     assert_basic_application()
 
-def test_node_application_options(wait_for_record):
+def test_node_application_options():
     client.load('options')
 
     assert_basic_application()
-    assert wait_for_record(r'constructor was called with unsupported') is not None
 
 
 def test_node_application_loader_unit_http():

callahad · 2024-02-08T20:40:17Z

What version of Node is in use on RHEL 8?

callahad · 2024-02-08T20:45:21Z

Whatever it is, I'd expect require('process') to work reliably everywhere.

andrey-zelenkov · 2024-02-08T21:37:26Z

What version of Node is in use on RHEL 8?

Sorry, forgot most important part:

./configure nodejs
...
configuring nodejs module
checking for node ... found
 + node version v10.24.0
checking for npm ... found
 + npm version 6.14.11
checking for node-gyp ... found
 + node-gyp version v9.4.0

callahad · 2024-02-08T22:14:59Z

Awesome, thank you for providing that :) Using require('process') instead of require('node:process') will work on Node 10. Technically, the require() call isn't necessary on any version, as process is a Node.js global, but it doesn't hurt to call it.

Separately, we should consider this case when discussing supported platforms. Node 10 has been end-of-life for nearly three years, and even RedHat won't provide paid support for it (or v12 or v14) on RHEL. For those customers, the oldest commercially supported version is Node 16 on RHEL 8.7 or greater.

andrey-zelenkov · 2024-02-08T22:37:58Z

Whatever it is, I'd expect require('process') to work reliably everywhere.

That works!

ac000 · 2024-02-14T14:16:17Z

For the first patch please put the Closes: (with a `:``) tag at the bottom of the commit message, I.e

Take options as well as requestListener

Unit-http have not kept up with the signature of nodejs's http package
development. Nodejs allows an optional `options` object to be passed to
the `createServer` function, we didn't. This resulted in function
signature errors when user code that did make use of the options arg
tried to call unit's replaced function.

This change changes the signature to be more in line with how nodejs
does it discarding it and printing a message to stdout.

Closes: https://github.com/nginx/unit/issues/1043

Unit-http have not kept up with the signature of nodejs's http package development. Nodejs allows an optional `options` object to be passed to the `createServer` function, we didn't. This resulted in function signature errors when user code that did make use of the options arg tried to call unit's replaced function. This change changes the signature to be more in line with how nodejs does it discarding it and printing a message to stdout. Closes: nginx#1043

callahad · 2024-02-19T12:35:35Z

Heh, during all the discussion about process.stderr I somehow glossed over the fact that we could just call console.warn, and it'd arguably be slightly more idiomatic. Suggested that in #1135

* Take options as well as requestListener Unit-http have not kept up with the signature of nodejs's http package development. Nodejs allows an optional `options` object to be passed to the `createServer` function, we didn't. This resulted in function signature errors when user code that did make use of the options arg tried to call unit's replaced function. This change changes the signature to be more in line with how nodejs does it discarding it and printing a message to stdout. * Add test file to start node application with options * Add changes to docs/changes.xml Closes: #1043

This reverts commit 21263d1.

* Take options as well as requestListener Unit-http have not kept up with the signature of nodejs's http package development. Nodejs allows an optional `options` object to be passed to the `createServer` function, we didn't. This resulted in function signature errors when user code that did make use of the options arg tried to call unit's replaced function. This change changes the signature to be more in line with how nodejs does it discarding it and printing a message to stdout. * Add test file to start node application with options * Add changes to docs/changes.xml Closes: nginx#1043

javorszky force-pushed the gabor/issue-1043 branch from c0df715 to 48421f8 Compare January 25, 2024 15:44

callahad reviewed Jan 25, 2024

View reviewed changes

src/nodejs/unit-http/http.js Outdated Show resolved Hide resolved

ac000 requested changes Jan 25, 2024

View reviewed changes

src/nodejs/unit-http/http.js Outdated Show resolved Hide resolved

javorszky force-pushed the gabor/issue-1043 branch from 48421f8 to 52550b1 Compare January 26, 2024 09:19

ac000 reviewed Jan 26, 2024

View reviewed changes

src/nodejs/unit-http/http.js Outdated Show resolved Hide resolved

andrey-zelenkov reviewed Jan 26, 2024

View reviewed changes

test/node/options/app.js Outdated Show resolved Hide resolved

docs/changes.xml Outdated Show resolved Hide resolved

javorszky force-pushed the gabor/issue-1043 branch from cdf9bfe to e14ed7d Compare January 29, 2024 11:31

callahad reviewed Jan 29, 2024

View reviewed changes

src/nodejs/unit-http/http_server.js Outdated Show resolved Hide resolved

javorszky force-pushed the gabor/issue-1043 branch 2 times, most recently from db4691e to 7926d1e Compare January 29, 2024 12:08

ac000 previously approved these changes Jan 30, 2024

View reviewed changes

andrey-zelenkov requested changes Jan 30, 2024

View reviewed changes

test/test_node_application.py Outdated Show resolved Hide resolved

src/nodejs/unit-http/http_server.js Show resolved Hide resolved

src/nodejs/unit-http/http_server.js Outdated Show resolved Hide resolved

javorszky force-pushed the gabor/issue-1043 branch from 7926d1e to 6f624dd Compare February 8, 2024 13:40

javorszky force-pushed the gabor/issue-1043 branch from ceb564c to e96eabc Compare February 8, 2024 15:42

andrey-zelenkov approved these changes Feb 8, 2024

View reviewed changes

javorszky force-pushed the gabor/issue-1043 branch from e96eabc to 3084881 Compare February 14, 2024 13:01

javorszky added 2 commits February 14, 2024 16:49

Add test file to start node application with options

2e8f1f9

Add changes to docs/changes.xml

9dafbdc

javorszky force-pushed the gabor/issue-1043 branch from 3084881 to 9dafbdc Compare February 14, 2024 16:49

ac000 self-requested a review February 14, 2024 17:28

ac000 approved these changes Feb 14, 2024

View reviewed changes

javorszky merged commit fbeb206 into nginx:master Feb 14, 2024

javorszky deleted the gabor/issue-1043 branch February 14, 2024 18:16

callahad added a commit that referenced this pull request Feb 27, 2024

Revert "fix: Take options as well as requestListener (#1091)"

4f2c947

This reverts commit 21263d1.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Take options as well as requestListener #1091

fix: Take options as well as requestListener #1091

javorszky commented Jan 25, 2024 •

edited

Loading

javorszky commented Jan 25, 2024 •

edited

Loading

javorszky commented Jan 26, 2024

ac000 commented Jan 30, 2024

javorszky commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

callahad commented Feb 8, 2024

callahad commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

callahad commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

ac000 commented Feb 14, 2024

callahad commented Feb 19, 2024

fix: Take options as well as requestListener #1091

fix: Take options as well as requestListener #1091

Conversation

javorszky commented Jan 25, 2024 • edited Loading

javorszky commented Jan 25, 2024 • edited Loading

javorszky commented Jan 26, 2024

ac000 commented Jan 30, 2024

javorszky commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

callahad commented Feb 8, 2024

callahad commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

callahad commented Feb 8, 2024

andrey-zelenkov commented Feb 8, 2024

ac000 commented Feb 14, 2024

callahad commented Feb 19, 2024

javorszky commented Jan 25, 2024 •

edited

Loading

javorszky commented Jan 25, 2024 •

edited

Loading