net: ensure net.connect calls Socket connect #12861

watson · 2017-05-05T14:27:00Z

This is an alternative PR to #12852 that avoids reverting bb06add.

Background

It's important for people who monkey-patch Socket.prototype.connect that it's called by net.connect since it's not possible to monkey-patch net.connect directly (as the connect function is called directly by other parts of lib/net.js instead of calling the exports.connect function).

Among the actors who monkey-patch Socket.prototype.connect are most APM vendors, the async-listener module and the continuation-local-storage module.

Checklist

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

Affected core subsystem(s)

net

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - nodejs#12342 - nodejs#12852

jkrems · 2017-05-07T18:05:40Z

I'm not sure if it's this PR or something else but I'm seeing the following when using continuation-local-storage against this branch:

> require('continuation-local-storage').createNamespace('foo').run(() => require('net').connect(11211))
{}
> Error: connect EADDRNOTAVAIL 127.0.0.1 - Local (0.0.0.0:64550)
    at Object.exports._errnoException (util.js:1026:11)
    at exports._exceptionWithHostPort (util.js:1049:20)
    at internalConnect (net.js:910:16)
    at emitLookup (net.js:1042:7)
    at /path/to/node_modules/async-listener/glue.js:188:31
    at GetAddrInfoReqWrap.asyncCallback [as callback] (dns.js:83:16)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:99:10)

Might be an unrelated issue? Doesn't happen w/o CLS. I was using latest async-listener & CLS.

sam-github · 2017-05-08T16:04:53Z

This has been approved, and around for 3 days, so it could be landed, but I'm concerned about @jkrems comment on c-l-s above. @jkrems Is it a regression? Does c-l-s fail on the commit before this one? @watson thoughts?

jkrems · 2017-05-08T16:20:42Z

Does c-l-s fail on the commit before this one?

Just tested - EADDRNOTAVAIL doesn't happen with this commit's parent (bc05436). Will do some more digging.

jkrems

With this change it's possible to make async-listener work again, 👍 for shipping it.

sam-github · 2017-05-08T17:35:03Z

@jkrems Ok, so the c-l-s thing you mentioned in #12861 (comment) is not a problem?

jkrems · 2017-05-08T17:41:56Z

@sam-github Well, it's not like c-l-s was working before this PR, it was just broken in different ways. I don't think this PR makes node#master compatible with the current c-l-s code but it makes it possible to fix c-l-s itself. And "broken but fixable" is better than "broken and impossible to fix". :)

sam-github · 2017-05-08T17:47:19Z

@jkrems OK, will you land? Or will @watson? I'm a bit confused, you both have "Member" tags, but only your name is on https://github.com/nodejs/node#collaborators. Generally, the PR author lands if they are a collaborator, so I don't want to land if Thomas can.

jkrems · 2017-05-08T17:58:27Z

New CI just in case: https://ci.nodejs.org/job/node-test-pull-request/7945/

~~I can give landing this a shot (if my IRC client decides to connect eventually...).~~

watson · 2017-05-08T20:47:46Z

@sam-github I don't have commit bit, so I don't think I can land it personally.

@jkrems I opened a tracking issue a few days ago on c-l-s to let people know about this problem: othiym23/node-continuation-local-storage#117

sam-github · 2017-05-08T20:58:12Z

Landed in 212a7a6

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - #12342 - #12852 PR-URL: #12861 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - nodejs#12342 - nodejs#12852 PR-URL: nodejs#12861 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

watson · 2017-06-15T12:01:23Z

I'd really love to have this backported to 7.x as well. Is it @MylesBorins or @jasnell that's responsible for backporting?

gibfahn · 2017-06-15T12:20:18Z

I'd really love to have this backported to 7.x as well. Is it @MylesBorins or @jasnell that's responsible for backporting?

It's @nodejs/backporting these days.

We're not planning to do another 7.x release unless there's a really good reason to (it's EoL). Is there a reason you specifically wanted it on 7.x?

watson · 2017-06-15T13:06:42Z

@gibfahn Thanks for the reply 😃 I appreciate the situation, but here's my situation (FYI: my frustrations are probably leaking though, so don't take any of it personal 😉):

This was discovered before 8 was out, so it was actually my understanding from talking to @MylesBorins that it would be included in a 7.x release, but apparently none was made 😬

My main concern is that a lot of CI's are failing because of this as people still test against 7. And people are probably still running 7 in the wild and are therefore affected by this (though hopefully they will upgrade to 8 over time as it gets more stable). This is a lot of time wasted debugging for a lot of people.

The CI builds for Node.js 7 for both async-listener, continuation-local-storage and opbeat all fail because of this which means we're flying blind. And most likely a lot of others, so It's very disrupting (see for instance this comment: othiym23/async-listener#113 (comment)).

I of course understand that we don't wanna make updates to a branch that's now EoL, but it's sad to leave 7.x in a state that breaks so much of the community. From a personal perspective getting this into 7 will make my life a lot easier.

As I said, it's not my intention to offend anybody or the current process 😅

gibfahn · 2017-06-15T13:11:33Z

Thanks @watson , that's the reason I was looking for.

7.x releases have mainly been done by @evanlucas and @cjihrig AIUI (but it could be done by anyone in @nodejs/release).

As I said, it's not my intention to offend anybody or the current process 😅

Doesn't come across as offensive, the current process is that we won't release unless there's a good reason, seems like you're providing a good reason so that's helpful.

evanlucas · 2017-06-15T13:50:13Z

I agree that there should be another v7.x release. I just frankly haven't had the time to do so.

watson · 2017-06-15T13:55:27Z

@evanlucas Let me know if there's anything I can do to help or make the process easier 😃

jasnell · 2017-06-15T14:00:19Z

I think a final 7.x release makes sense

gibfahn · 2017-06-20T22:47:02Z

#12342 is dont-land, so marking the same for this. LMK if you disagree.

watson · 2017-07-11T11:02:59Z

Will this land in the planned security release for 7.x that's coming out this week? Please, please, please 😅

gibfahn · 2017-07-11T11:43:53Z

Will this land in the planned security release for 7.x that's coming out this week? Please, please, please

I guess cc/ @mhdawson

MylesBorins · 2017-07-11T13:23:34Z

We are not planning to include any commits unrelated to security in the upcoming release

…

On Jul 11, 2017 12:44 PM, "Gibson Fahnestock" ***@***.***> wrote: Will this land in the planned security release for 7.x that's coming out this week? Please, please, please I guess cc/ @mhdawson <https://github.com/mhdawson> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12861 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVwsLraXlWBuPAkEdenOBYPEw-Mn5ks5sM1__gaJpZM4NR_VT> .

sam-github · 2017-07-11T16:48:25Z

@watson See https://github.com/nodejs/LTS/blob/master/doc/meetings/2017-05-15.md#clarify-what-happens-with-odd-numbered-releases-in-april-128 for some notes

7.x has no support, it was debated whether it should ever get any updates, for any reason (including security), after 8.x was available, and I think we settled on "ok, maybe for a really minimal amount of time, but no promises for the future". And no one should have been using 7.x for production, they are just one step more stable than nightly builds (not very stable). Which of course doesn't help you if one of your customers happens to be doing just that :-(.

Maybe comment on the LTS issue if you have some thoughts on what to do when we come out with a new major, with some feedback on what your customers are doing and impact on them.

watson · 2017-07-11T17:10:45Z

@sam-github did you see #12861 (comment)? I understod from @evanlucas that this was one of those exceptions?

sam-github · 2017-07-11T17:25:13Z

@watson I don't claim to perfect understanding, but my reading of what @evanlucas said is that he personally wants to do a 7.x release, and I don't think anyone would object if he wanted to, but I'm not sure the LTS WG is comitted to that. I think its more out of his personal interest. I think opening an issue in the LTS issue tracker to request another 7.x might be the best way of getting a definitive yes/no on a 7.x patch/bug fix release.

evanlucas · 2017-07-11T17:55:04Z

@watson I apologize. My original intention was to include this in the v7.10.1 release, but it slipped my mind during preparation. Sorry

watson · 2017-07-12T08:21:50Z

No worries. I'll open up an issue in the LTS issue tracker 👍

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label May 5, 2017

sam-github suggested changes May 5, 2017

View reviewed changes

watson force-pushed the net-connect branch from 148f3d8 to ee7cce1 Compare May 5, 2017 15:07

lucamaraschi approved these changes May 5, 2017

View reviewed changes

cjihrig approved these changes May 6, 2017

View reviewed changes

sam-github reviewed May 6, 2017

View reviewed changes

sam-github suggested changes May 6, 2017

View reviewed changes

sam-github approved these changes May 6, 2017

View reviewed changes

addaleax approved these changes May 6, 2017

View reviewed changes

addaleax reviewed May 6, 2017

View reviewed changes

jasnell approved these changes May 7, 2017

View reviewed changes

watson force-pushed the net-connect branch from ee7cce1 to ab7457a Compare May 7, 2017 10:20

watson mentioned this pull request May 7, 2017

Revert "net: don't normalize twice in Socket#connect()" #12852

Closed

2 tasks

jkrems approved these changes May 8, 2017

View reviewed changes

This was referenced May 8, 2017

net.connect issue with Node.js 7.10.0 othiym23/async-listener#109

Open

net.connect issue with Node.js 7.10.0 othiym23/node-continuation-local-storage#117

Open

sam-github closed this May 8, 2017

watson deleted the net-connect branch May 8, 2017 20:58

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

watson mentioned this pull request Jun 15, 2017

Makes WrappedPromise an ES6 class othiym23/async-listener#113

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added dont-land-on-v4.x labels Jun 20, 2017

watson mentioned this pull request Jan 8, 2018

Drop official support for Node.js v5 and v7? elastic/apm-agent-nodejs#158

Closed

+                // If passed an array, it's treated as an array of arguments that have
+                // already been normalized (so we don't normalize more than once). This have
+                // been solved before in https://github.com/nodejs/node/pull/12342, but was
+                // reverted as it had uninteded side effects.

		@@ -0,0 +1,39 @@
		'use strict';

		// This test checks that calling `net.connect` internally calls

+              const net = require('net');
+              const Socket = net.Socket;
+              // monkey patch Socket.prototype.connect to check that it's called

net: ensure net.connect calls Socket connect #12861

net: ensure net.connect calls Socket connect #12861

Conversation

watson commented May 5, 2017

Background

Checklist

Affected core subsystem(s)

Related

sam-github left a comment

Choose a reason for hiding this comment

sam-github May 5, 2017

Choose a reason for hiding this comment

sam-github May 5, 2017

Choose a reason for hiding this comment

sam-github May 5, 2017

Choose a reason for hiding this comment

Fishrock123 commented May 5, 2017

mscdex commented May 5, 2017

lucamaraschi commented May 5, 2017

watson commented May 6, 2017

sam-github May 6, 2017

Choose a reason for hiding this comment

watson May 7, 2017

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

sam-github commented May 6, 2017

addaleax May 6, 2017

Choose a reason for hiding this comment

MylesBorins May 7, 2017

Choose a reason for hiding this comment

jkrems commented May 7, 2017 • edited Loading

sam-github commented May 8, 2017

jkrems commented May 8, 2017

jkrems left a comment

Choose a reason for hiding this comment

sam-github commented May 8, 2017

jkrems commented May 8, 2017

sam-github commented May 8, 2017

jkrems commented May 8, 2017 • edited Loading

watson commented May 8, 2017

sam-github commented May 8, 2017

watson commented Jun 15, 2017

gibfahn commented Jun 15, 2017

watson commented Jun 15, 2017 • edited Loading

gibfahn commented Jun 15, 2017

evanlucas commented Jun 15, 2017

watson commented Jun 15, 2017

jasnell commented Jun 15, 2017

gibfahn commented Jun 20, 2017

watson commented Jul 11, 2017

gibfahn commented Jul 11, 2017

MylesBorins commented Jul 11, 2017 via email

sam-github commented Jul 11, 2017

watson commented Jul 11, 2017

sam-github commented Jul 11, 2017

evanlucas commented Jul 11, 2017

watson commented Jul 12, 2017

jkrems commented May 7, 2017 •

edited

Loading

jkrems commented May 8, 2017 •

edited

Loading

watson commented Jun 15, 2017 •

edited

Loading