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

[v11.x backport] http: reduce usage of public util #26650

Closed
wants to merge 91 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 14, 2019

PR-URL: #26548
Refs: #26546
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: James M Snell [email protected]
Reviewed-By: Colin Ihrig [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Sorry, something went wrong.

mcollina and others added 30 commits March 12, 2019 21:04

Unverified

The email in this signature doesn’t match the committer email.
Fixes: nodejs#25810

PR-URL: nodejs#26059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#26395
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: nodejs#26366

PR-URL: nodejs#26402
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#26380
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
We need to be careful not to include the size of non-pointer
fields in the parent's self size if we want to track them separately
as a different node.

Refs: https://github.com/nodejs/node/pull/26161/files#r259170771

PR-URL: nodejs#26262
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Edit the last few paragraphs of the Collaborator Guide section on
deprecations.

PR-URL: nodejs#26419
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The underlying JavaScript runtime may schedule tasks at its discretion
so there may be more timer handles than the one created by the test.

PR-URL: nodejs#26434
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Migrate various modules from using process.binding to internalBinding.

PR-URL: nodejs#24952
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Expose the size of asymetric keys of crypto key object from the
crypto module added in v11.6.0.

PR-URL: nodejs#26387
Refs: nodejs#24234
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Instead of setting it in the process object, since this is
a configure-time option. Also added a shim that can be
deprecated and removed some time later.

PR-URL: nodejs#26228
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26430
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#26429
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26426
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26398
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26398
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26431
Refs: nodejs#25994
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
If run without `--verbose` configure exits silently with no indication
that it has done anything. Print a message on completion to indicate
that the script has worked.

Refs: nodejs#23111

PR-URL: nodejs#26436
Refs: nodejs#23111
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Do not write one character too much before shifting the whole result
to the left when using UTF16-LE, possibly overwriting already-used
memory while doing so.

Fixes: nodejs#26422

PR-URL: nodejs#26432
Fixes: nodejs#26422
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
PR-URL: nodejs#26433
Refs: nodejs#2228
Refs: nodejs#4252
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Use "End-of-Life" everywhere and not "End-of-life" or "End-Of-Life".

PR-URL: nodejs#26442
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
This update includes an additional check for `End-of-life`. For
consistency, we use `End-of-Life` everywhere.

PR-URL: nodejs#26442
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
PR-URL: nodejs#26427
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#26397
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26407
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Use the `cwd` option for child_process instead of `process.chdir()` to
allow tests to work with worker threads.

PR-URL: nodejs#26453
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Prefer `internalBinding` or other equivalents over `process.binding()`
(except in tests checking `process.binding()` itself).

PR-URL: nodejs#26304
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#26454
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
PR-URL: nodejs#26446
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
This reverts commit 0373836.

PR-URL: nodejs#26358
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax and others added 8 commits March 13, 2019 19:13
Destroy the `Worker` class earlier, because we don’t need access
to it once the thread has stopped and all resources have been
cleaned up.

PR-URL: nodejs#26542
Fixes: nodejs#26535
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#26538
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26547
Refs: nodejs#26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#26319
Fixes: nodejs#26316
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Fixes a bug I introduced in 9613221

PR-URL: nodejs#26570
Refs: nodejs#25870
Refs: nodejs#26473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: nodejs#26488
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#24346
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
If this file is loaded with Node.js build without ssl, it will load
'node-inspect/lib/internal/inspect_client' as well. Due to that two
deprecation warnings will be emitted instead of one and the test
fails.

It should be safe to just test all other cases and to ignore this
specific file.
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. v11.x labels Mar 14, 2019
@refack refack mentioned this pull request Mar 14, 2019
2 tasks
@refack
Copy link
Contributor Author

refack commented Mar 14, 2019

PR-URL: nodejs#26548
Refs: nodejs#26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@refack refack force-pushed the backport-26548-to-v11.x branch from d1f1c12 to cd77811 Compare March 14, 2019 02:58
@refack
Copy link
Contributor Author

refack commented Mar 14, 2019

So it seems this is dependent on #24755, but I'm not sure if the specific changes here are exposed in the API, PTAL.

/CC @BridgeAR @mcollina @lpinca @joyeecheung (who reviewed #24755)

@refack
Copy link
Contributor Author

refack commented Mar 14, 2019

@@ -106,7 +105,8 @@ function Agent(options) {
});
}

util.inherits(Agent, EventEmitter);
Copy link
Member

@ZYSzys ZYSzys Mar 14, 2019

Choose a reason for hiding this comment

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

Since #24755 was semver-major, maybe we should keep using util.inherits here and just backport the debuglog part ?

BTW, I think it's also dependent on #26468 if I'm not wrong.

Copy link
Member

Choose a reason for hiding this comment

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

This definitely depends on #26468 and it's also the reason why the CI fails.

I have no strong opinion about the util.inherits part. I guess someone could rely on some checks that would now fail but it is quite unlikely as well. In this case I would just follow @ZYSzys and pull these changes out.

@BridgeAR BridgeAR force-pushed the v11.x-staging branch 4 times, most recently from f95e015 to 1fa5004 Compare March 14, 2019 16:15
targos pushed a commit that referenced this pull request Mar 28, 2019
Backport-PR-URL: #26650
PR-URL: #26548
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos
Copy link
Member

targos commented Mar 28, 2019

Landed in 04441b5 without the util.inherits changes.

@targos targos closed this Mar 28, 2019
targos pushed a commit that referenced this pull request Mar 30, 2019
Backport-PR-URL: #26650
PR-URL: #26548
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@refack refack deleted the backport-26548-to-v11.x branch April 14, 2019 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet