fix: Take options as well as requestListener

[javorszky]

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 stderr so it shows up in unit logs.

[javorszky]

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.

[javorszky]

@andrey-zelenkov I added the docs to changes in https://github.com/nginx/unit/pull/1091/commits/cdf9bfe32d17925f56d50b82e19ff7a45b094b71 as a response to your comment on the issue here: https://github.com/nginx/unit/issues/1013#issuecomment-1912149102

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

[ac000]

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>

[javorszky]

@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]

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]

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]

What version of Node is in use on RHEL 8?

[callahad]

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

[andrey-zelenkov]

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]

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]

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

That works!

[ac000]

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

[callahad]

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