Revert "net: don't normalize twice in Socket#connect()" #12852
Conversation
This reverts commit bb06add. This commit was causing failures in the async-listener module, which monkey patches Socket.prototype.connect().
|
Would it be okay if we only reverted this in v7.x? |
|
Let's definitely revert on v7. Talking this over with @watson, one approach moving forward to avoid normalizing twice could be to detect when an array is passed. None of the public API signatures accept an array, but that is what is returned from |
|
I'm currently working on a test to show this PR works. Afterwards I'll open a PR for passing the normalized args as an array from |
This is important for people who monkey-patches `Socket.prototype.connect` since it's not possible to monkey-patch `net.connect` directly (as the core `connect` function is called internally in Node instead of calling the `exports.connect` function). Monkey-patching of `Socket.prototype.connect` is done by - among others - most APM vendors, the async-listener module and the continuation-local-storage module.
|
Regression test added, PTAL. |
|
I don't think it's okay to lock down our internal implementation for the benefit of third-party code. I'm alright with reverting this in v7.x but for 8.0 @watson should fix this in async-listener. |
|
If we still accept arrays in 8.x this behavior should be documented in case
people bump into weird bugs. (Or this needs to be documented at least in
the changelog anyway)
…On Sun, 7 May 2017 at 9:07 AM Ben Noordhuis ***@***.***> wrote:
I don't think it's okay to lock down our internal implementation for the
benefit of third-party code. I'm alright with reverting this in v7.x but
for 8.0 @watson <https://github.com/watson> should fix this in
async-listener.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#12852 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEGanJjYdMph7vgnwM1OrFGYD1cZx_Ffks5r3W3CgaJpZM4NRs4E>
.
|
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
|
@bnoordhuis I totally see your point and agree this isn't nice at all. I'd wish there were a better way to support this use-case. I would have fixed this in userland if I could, but unfortunately I can't just monkey-patch Being able to trace outgoing tcp sockets is a huge deal for large parts of the Node community, so it's something we'll have to support. So far monkey-patching have been the way this was done from inside the process, so if we refactor the internals of In the long run I like to see an API that could be exposed by core so that monkey-patching would no longer be necessary. For now I think #12861 is a good middle ground because it avoids reverting bb06add while not breaking the existing ecosystem 😃 |
|
Seconded, our internal instrumentation code also broke. Not sure if it would be possible to offer some official API/hook for achieving the same thing. But until that exists (or this change is reverted), we won't be able to test node 8 with our apps. :( EDIT: Nvm, looks like #12861 is already on its way into master. |
|
Closing this in favor of #12861 |
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>
This reverts commit bb06add. This commit was causing failures in the async-listener module, which monkey patches
Socket.prototype.connect().cc: @watson @jasnell
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
net