Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: ensure net.connect calls Socket connect #12861

Closed
wants to merge 1 commit into from

Conversation

@watson
Copy link
Member

commented May 5, 2017

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

cc: @cjihrig @addaleax @jasnell

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
Related
@sam-github
Copy link
Member

left a comment

LGTM, though a couple cosmetic requests.

/cc @tobespc

lib/net.js Outdated

let normalized;
// 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

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

"This has"

lib/net.js Outdated
// 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.

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

"unintended"

test/parallel/test-net-connect-call-socket-connect.js Outdated
@@ -0,0 +1,39 @@
'use strict';

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

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

I don't know why it was chosen, but the standard is for the test description to be later in the file.

@watson watson force-pushed the watson:net-connect branch May 5, 2017

@Fishrock123

This comment has been minimized.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

@lucamaraschi

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

LGTM

@cjihrig
cjihrig approved these changes May 6, 2017
@watson

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

@sam-github I think I addressed all your review point 😃

test/parallel/test-net-connect-call-socket-connect.js Outdated
const net = require('net');
const Socket = net.Socket;

// monkey patch Socket.prototype.connect to check that it's called

This comment has been minimized.

Copy link
@sam-github

sam-github May 6, 2017

Member

Style is to capitalize sentence and add a period. Sorry for the nitpicking.

This comment has been minimized.

Copy link
@watson

watson May 7, 2017

Author Member

@sam-github np 😄 Just amended the commit with a fix 😃

@sam-github
Copy link
Member

left a comment

LGTM, though one sentence could be capitalized and punctuated.

@sam-github

This comment has been minimized.

Copy link
Member

commented May 6, 2017

I think I addressed all your review point

You did, thank you.

// - https://github.com/nodejs/node/pull/12852

const net = require('net');
const Socket = net.Socket;

This comment has been minimized.

Copy link
@addaleax

addaleax May 6, 2017

Member

Actually, feel free to just go with const { Socket } = require('net'); if you like.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins May 7, 2017

Member

net.connect is used as well.

@jasnell
jasnell approved these changes May 7, 2017
net: ensure net.connect calls Socket connect
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

@watson watson force-pushed the watson:net-connect branch to ab7457a May 7, 2017

@watson watson referenced this pull request May 7, 2017
2 of 2 tasks complete
@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 7, 2017

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

This comment has been minimized.

Copy link
Member

commented May 8, 2017

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

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

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
jkrems approved these changes May 8, 2017
Copy link
Contributor

left a comment

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

@sam-github

This comment has been minimized.

Copy link
Member

commented May 8, 2017

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

@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

@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

This comment has been minimized.

Copy link
Member

commented May 8, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

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

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

@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

This comment has been minimized.

Copy link
Member

commented May 8, 2017

Landed in 212a7a6

@sam-github sam-github closed this May 8, 2017

sam-github added a commit that referenced this pull request May 8, 2017
net: ensure net.connect calls Socket connect
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>

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

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
net: ensure net.connect calls Socket connect
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>
@jasnell jasnell referenced this pull request May 11, 2017
@watson

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2017

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

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

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

@watson

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

I think a final 7.x release makes sense

@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

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

@watson

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

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

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

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

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.