[4.x] http: make socketPath work with no agent

[lpinca]

This is a backport of #19425.

Currently Agent.prototype.createConnection() is called uncoditionally
if the socketPath option is used. This throws an error if no agent is
used, preventing, for example, the socketPath and createConnection
options to be used together.

This commit fixes the issue by falling back to the createConnection
option or net.createConnection().

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[lpinca]

CI: https://ci.nodejs.org/job/node-test-pull-request/13778/

[lpinca]

The next 4.x release will probably be the final one. It would be nice to include this fix.
cc: @nodejs/http @nodejs/lts.

[MylesBorins]

@lpinca the next planned release is a security release that will only include those changes.

We can do one more release, but I am genuinely nervous about landing anything... even if it "improves things". Last minute behavior changes, even those that fix broken behavior, have the potential of breaking code in production. I would very much like to avoid a situation where we do a release in the weeks before the EOL that requires us to have to backtrack

/cc @nodejs/lts

[lpinca]

It's a very simple fix for a combination of options (options.socketPath + options.createConnection) that has practically no usage, no one reported the issue in ~3 years but I understand, feel free to close if you think it's too risky.

[gibfahn]

I think I agree with @MylesBorins on this one, ideally we'd like to make no changes to 4.x at all at this point, so the bar for "worth adding" is very high. I think the only changes we should be considering are security issues and build/test fixes that we need to get the release out.

no one reported the issue in ~3 years but I understand, feel free to close if you think it's too risky.

For me it's not just the risk, it's that by actively updating the 4.x line at this point we are giving the impression that 4.x is still updated so it's still okay to use. If people are hitting bugs, they should update to 6.x.

[lpinca]

Well by cutting a new 4.x release we are actively updating it. No new features ok, but why not bug fixes, it's still officially supported.

[MylesBorins]

@lpinca the release coming tuesday will be a security release. For LTS releases we do not include anything in the release aside from the security patch.

So this would involve doing another v4.x release, which would likely have to be the week before going EOL if we wanted to do an R.C.

While it is a bug fix, it still can be a change in behavior. For an EOL branch I would rather advise people to upgrade for the fix.

Is this bugfix considered critical? Generally we only do releases for critical bug fixes in Maintenance branches

[gibfahn]

it's still officially supported.

It's officially in maintenance, which means we support it but you should be upgrading ASAP. If this has been an issue for the life of 4.x, then I would say that it's reasonable to expect anyone who wants this fix to be upgrading to 6.x.

[lpinca]

While it is a bug fix, it still can be a change in behavior.

I doubt anyone is relying on this behavior when options.createConnection is set 😄:

Uncaught TypeError: Cannot read property 'createConnection' of undefined
      at new ClientRequest (_http_client.js:129:29)
      at Object.exports.request (http.js:31:10)
      at Object.exports.get (http.js:35:21)
      at WebSocket.initAsClient (lib/websocket.js:121:4457)
      at new WebSocket (lib/websocket.js:12:1247)
      at Server.<anonymous> (test/websocket-server.test.js:130:20)
      at emitListeningNT (net.js:1279:10)

It's not a critical bug fix, as said it seems no one but ws is using socketPath + createConnection.

I'm going to close this and resort to horrible hacks to fix the issue in ws.
Thanks for the feedback.

[lpinca]

I would say that it's reasonable to expect anyone who wants this fix to be upgrading to 6.x.

Which is also affected by the same bug, so they should jump from 4.x to 8.x

[MylesBorins]

6.x can still be fixed without an issue

…

On Mar 22, 2018, at 4:20 PM, Luigi Pinca ***@***.***> wrote: I would say that it's reasonable to expect anyone who wants this fix to be upgrading to 6.x. Which is also affected by the same bug, so they should jump from 4.x to 8.x — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19489 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVyftm_cF3_9C3PnTQdZe6Ryy2jPzks5tg89QgaJpZM4SyYMq>.