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

[v8.x] https: revert "refactor to use http internals" #16660

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Nov 1, 2017

This reverts commit 5118f31 on the v8.x release line

It is breaking code in the wild that depends on the original behavior
to do tracing.

I don't think we need to necessarily fix this in 9.x but we might want
to reclassify the original commit as Semver Major

Refs: #16395

/cc @nodejs/lts @bengl @gibfahn @ofrobots

@nodejs-github-bot nodejs-github-bot added https Issues or PRs related to the https subsystem. v8.x labels Nov 1, 2017
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Nov 1, 2017

@addaleax
Copy link
Member

addaleax commented Nov 1, 2017

Is there a bug report of some kind somewhere?

Also, our release tooling is designed to handle git’s built-in revert commit messages and explicitly calls out the commits as reverts if it is used.

This reverts commit 5118f31.

It is breaking code in the wild that depends on the original behavior
to do tracing.

I don't think we need to necessarily fix this in 8.x but we might want
to reclassify the original commit as Semver Major

Refs: nodejs#16395
@MylesBorins
Copy link
Contributor Author

@addaleax There are two comments found at #16395 (comment)

lmk if commit message looks good

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, seems good for v8.x!

@MylesBorins MylesBorins changed the title https: revert "refactor to use http internals" [v8.x] https: revert "refactor to use http internals" Nov 1, 2017
@refack
Copy link
Contributor

refack commented Nov 1, 2017

I don't think we need to necessarily fix this in 8.x but we might want
to reclassify the original commit as Semver Major

I'm assuming you meant 9?

@MylesBorins
Copy link
Contributor Author

@refack yeah, I just updated that in the commit message 😅

Waiting for CI to run before pushing because I don't want to restart it again

@refack
Copy link
Contributor

refack commented Nov 1, 2017

P.S. any good candidate for CitGM in the failures?

@MylesBorins
Copy link
Contributor Author

@addaleax @bengl are you cool with the original commit being relabeled semver major? I'll update changelogs

@bnoordhuis
Copy link
Member

No reason to revert, IMO. We don't promise that internals won't change. People monkey-patching core APIs know and accept that.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis does it make sense to knowlingly break people in a patch for such a small optimization? Seems reasonable to give people some time to catch up

@bnoordhuis
Copy link
Member

They'll need to update anyway, that's the responsibility you take on when you start monkey-patching core.

There's also the principle of the matter. A hard-line stance avoids future debate.

@apapirovski
Copy link
Member

apapirovski commented Nov 1, 2017

Kinda -1 on reverting this for the same reasons @bnoordhuis is. Also, I don't think semver-major really applies to internal implementation details.

@jasnell
Copy link
Member

jasnell commented Nov 1, 2017

Our documented guidelines specifically call out the fact that we can change internals in ways that potentially break. So, yes, in this case semver-major would not apply technically. We have used in the the past, however, out of an abundance of caution to try to avoid breakage but I definitely have to say that I have sympathy for @bnoordhuis' position.

@evanlucas
Copy link
Contributor

While I understand the sentiment, I disagree that we shouldn't land this. It doesn't really cost us anything and will keep us from having people that cannot update to use the LTS version of node 8 now. Just my 2 cents though.

@ofrobots
Copy link
Contributor

ofrobots commented Nov 1, 2017

Normally we get feedback for a change on Current for a duration before potentially breaking changes land in LTS. That didn't happen here, which means that there was no warning that this was coming down the pipe before it broke folks that are building diagnostic tooling.

On the flip side, I don't think there is a great argument that this change should go into LTS to begin with. The code review already identified that this is potentially breaking. Restricting this to 9.x+ seems appropriate to me.

@jasnell
Copy link
Member

jasnell commented Nov 1, 2017

I'm fine with reverting in 8.x only

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most cases I strongly agree with core being able to change its own internals. In this case, I'm OK with reverting because this change was identified as possibly breaking and went straight into an LTS release with no baking period in Current.

@ofrobots
Copy link
Contributor

ofrobots commented Nov 1, 2017

@bnoordhuis @apapirovski This revert isn't about preventing core from being able to change internals. It is rather about not putting these changes directly into LTS without a baking period. Reverting it now helps avoid people running into breakages across the ecosystem. Does that formulation help remove your -1?

@bnoordhuis
Copy link
Member

@ofrobots I get your point but it doesn't change mine. I won't rehash my arguments and I won't fight it to death but I feel it sets a bad precedent.

As to the argument that a revert doesn't really cost anything: it makes back-porting fixes from the master branch harder. It's not free.

@refack
Copy link
Contributor

refack commented Nov 1, 2017

@MylesBorins ack on semver level.
The list of precedents makes a good case for formalizing this ad-hoc policy.
IMHO once we have a formal policy it's easier to draw the line in the sand on principal.

@MylesBorins
Copy link
Contributor Author

@refack I don't believe that we should make a formal policy around reverts aside from revert often and quickly.

@MylesBorins
Copy link
Contributor Author

Also for clarity, with the added information from this thread I see no reason for this change to be treated as Semver-Major. Semver-Patch makes the most sense

@bengl
Copy link
Member

bengl commented Nov 1, 2017

I'm -1 on calling this a semver-major, but +1 on the revert. I think the revert will probably help enough folks that it's worth it, but I certainly don't want to set a precedent that internal API changes are semver-major if they only break folks who are doing things with API internals.

Again: +1 on revert, simply for let's-unbreak-people purposes.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it explicit: I am +1 on reverting in v8.x if users are willing to fix other their side, but we should have a better channel to coordinate these changes, before async_hooks are leaves experimental stage there will still be apm vendors sensitive to the internal changes. cc @nodejs/diagnostics

@gireeshpunathil
Copy link
Member

I like the idea of: when arguments saturate, favor ecosystem and existing deployments.

monkey-patching seems to originate partly from the language semantics, and partly from the need for monitoring as an essential embodiment of modern workloads. I propose to address monitoring capability in the core as an important focus area.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik this change breaks the only reliable way to instrument outgoing HTTP request, so +1 on reverting and I'd actually say we shouldn't have this change anywhere w/o an official API to achieve the same thing.

@bnoordhuis
Copy link
Member

Afaik this change breaks the only reliable way to instrument outgoing HTTP request

It doesn't, you just need to update your code if you were monkey-patching that method.

Matter of fact, if this change broke your tracer it wasn't catching everything in the first place - requests made directly through the ClientRequest interface.

@jkrems
Copy link
Contributor

jkrems commented Nov 2, 2017

Matter of fact, if this change broke your tracer it wasn't catching everything in the first place - requests made directly through the ClientRequest interface.

Okay, maybe I should've said "most reliable way in practice I'm aware of". There's little if any code I ever ran across that was using the ClientRequest constructor directly. Anyhow, the point here is: Landing this directly in an LTS release isn't a great precedent. Thus my 👍 for a revert.

@gibfahn
Copy link
Member

gibfahn commented Nov 2, 2017

@refack I don't believe that we should make a formal policy around reverts aside from revert often and quickly.

@MylesBorins why?

@NatalieWolfe
Copy link

Matter of fact, if this change broke your tracer it wasn't catching everything in the first place - requests made directly through the ClientRequest interface.

Not necessarily. In our case we were explicitly not instrumenting the https module because the http instrumentation caught everything. The solution was very simple though, and it matters not to us if this is reverted. Heck, it's less work for me if it isn't. :)

MylesBorins added a commit that referenced this pull request Nov 3, 2017
This reverts commit 5118f31.

It is breaking code in the wild that depends on the original behavior
to do tracing.

I don't think we need to necessarily fix this in 8.x but we might want
to reclassify the original commit as Semver Major

PR-URL: #16660
Refs: #16395
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@MylesBorins
Copy link
Contributor Author

landed in 08b75c1

@MylesBorins MylesBorins closed this Nov 3, 2017
@gibfahn gibfahn mentioned this pull request Nov 5, 2017
gibfahn added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

- **openssl**:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) [#16691](#16691)
- ***Revert*** "**https**:
  - refactor to use http internals" (Myles Borins) [#16660](#16660)

PR-URL: #16783
gibfahn added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

- **openssl**:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) [#16691](#16691)
- ***Revert*** "**https**:
  - refactor to use http internals" (Myles Borins) [#16660](#16660)

PR-URL: #16783
@MylesBorins MylesBorins deleted the revert-16395 branch November 14, 2017 17:44
@danielkhan
Copy link

danielkhan commented Nov 15, 2017

Sorry for being late to the party. I like @MylesBorins pragmatic suggestion to add tooling that removes the need to monkey patch core. I also agree with @NatalieWolfe that async hooks should and could solve that going forward. We should take that as an use case for improvement in nodejs/diagnostics#117

mattcolegate pushed a commit to RuntimeTools/appmetrics that referenced this pull request Jan 15, 2018
As the previous change was reverted, and won't go into the 8.x release
line, we only need to do this for 9.0.0 and up.

Refs: nodejs/node#16660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.