tls,http,https: replace url.parse() with WHATWG URL parser#20270

Hackzzila · 2018-04-25T02:01:32Z

Replaces url.parse() with the WHATWG URL parser in the tls, http, and https modules.
Removes ERR_INVALID_DOMAIN_NAME, ERR_INVALID_URL will be thrown instead.

Fixes: #19468

Checklist

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

Fixes: nodejs#19468

devsnek · 2018-04-25T02:12:42Z

/cc @nodejs/url @nodejs/http

vsemozhetbyt · 2018-04-25T02:28:37Z

doc/api/http.md

 [`EventEmitter`]: events.html#events_class_eventemitter
 [`TypeError`]: errors.html#errors_class_typeerror
 [`URL`]: url.html#url_the_whatwg_url_api
+[`new URL()`]: url.html#url_constructor_new_url_input_base


A nit: it seems this should go after [`net.createConnection()`]: ABC-wise.

Whoops, fixed in the latest commit.

vsemozhetbyt · 2018-04-25T02:29:39Z

doc/api/https.md


 [`Agent`]: #https_class_https_agent
 [`URL`]: url.html#url_the_whatwg_url_api
+[`new URL()`]: url.html#url_constructor_new_url_input_base


A nit: it seems this should go after [`net.Server`]: ABC-wise.

Whoops, fixed in the latest commit.

vsemozhetbyt · 2018-04-25T02:44:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14483/

lpinca · 2018-04-25T05:55:28Z

Are there performance regressions or improvements?

Hackzzila · 2018-04-25T06:25:39Z

If I modify benchmark/url/legacy-vs-whatwg-url-parse.js to include the urlToOptions call, this is what I get.

url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="long": 289,075.9348944105 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="long": 139,096.9125718235 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="short": 769,774.774985596 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="short": 350,759.762306244 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="idn": 512,248.51248922833 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="idn": 386,065.3465710354 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="auth": 492,149.1264020806 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="auth": 443,939.43866965553 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="file": 725,138.3524093787 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="file": 385,119.7062769024 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="ws": 532,978.6339205448 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="ws": 442,048.80710388697 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="javascript": 1,252,020.322593553 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="javascript": 779,430.8874695955 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="percent": 846,338.6383362594 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="percent": 495,170.02492215496 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="legacy" type="dot": 725,815.0320343183 url\legacy-vs-whatwg-url-parse-options.js n=100000 method="whatwg" type="dot": 365,448.9973075362```

lpinca · 2018-04-25T06:30:20Z

Ok thank you, it seems the impact is big.

mscdex · 2018-04-25T10:44:43Z

I think we should work on minimizing the performance regression first.

joyeecheung · 2018-04-25T10:50:00Z

@Hackzzila You may want to provide an option to use the new parser and benchmark against http.request instead of url. The cost of switching the parser may be less significant in the context of http.request.

BridgeAR · 2018-04-25T11:00:43Z

I agree with @joyeecheung. Yes, it is slower to use the WHATWG URL parser in combination with urlToOptions but likely, this is not a major pain point overall when doing http.request. So I would like to see a benchmark and or a profile for that.

As a separate concern, we can try to improve the overall WHATWG URL parser performance.

TimothyGu · 2018-04-25T14:25:55Z

Aww darn, I had a local branch that had this change, but is slightly different in that it still maintains compatibility with the older URL parser. I just pushed it as a WIP to #20288, and I'd appreciate any help in getting it completed.

Hackzzila · 2018-04-25T14:47:39Z

I modified benchmark/http/create-clientrequest.js to pass in a string, and I disabled the last few lines of the CreateConnection constructer so it wouldn't make a connection.

WHATWG

custom-test\create-clientrequest.js n=1000000 len=1: 431,118.2280318964 custom-test\create-clientrequest.js n=1000000 len=8: 216,333.86282733665 custom-test\create-clientrequest.js n=1000000 len=16: 161,106.82357163524 custom-test\create-clientrequest.js n=1000000 len=32: 97,847.83486623652 custom-test\create-clientrequest.js n=1000000 len=64: 65,573.95854501659 custom-test\create-clientrequest.js n=1000000 len=128: 37,945.55008883177

url.parse

custom-test\create-clientrequest.js n=1000000 len=1: 602,444.7556396928 custom-test\create-clientrequest.js n=1000000 len=8: 592,540.2376684345 custom-test\create-clientrequest.js n=1000000 len=16: 546,397.7322527079 custom-test\create-clientrequest.js n=1000000 len=32: 469,340.2886209889 custom-test\create-clientrequest.js n=1000000 len=64: 375,066.31847630034 custom-test\create-clientrequest.js n=1000000 len=128: 267,812.752845535

There seems to be a big difference between the two. However, I have a few points.

http2 uses the WHATWG URL parser already
I don't see many people passing a string into ClientRequest anyways.
I feel that "correct" URL parsing is more important than performance (however it may be too much in this case)

Maybe this PR should wait until the WHATWG URL parser has better performance.

TimothyGu · 2018-04-25T15:35:57Z

I disabled the last few lines of the CreateConnection constructer so it wouldn't make a connection.

I don't think you should. I think @joyeecheung's intent is for us to measure how much time URL parsing takes relative to a network call, so that we can find out if we need to worry about URL parsing in this context at all.

lpinca · 2018-04-25T16:05:05Z

I don't think it's very useful because in that case you are not benchmarking URL parsing.

Hackzzila · 2018-04-25T16:21:47Z

Oh, I misunderstood. I ran some new benchmarks.

WHATWG

custom-test\client-request-body.js method="end" len=32 type="asc" dur=5: 2,616.1966389562353 custom-test\client-request-body.js method="end" len=256 type="asc" dur=5: 2,593.879979176502 custom-test\client-request-body.js method="end" len=1024 type="asc" dur=5: 2,607.689847236272 custom-test\client-request-body.js method="end" len=32 type="utf" dur=5: 2,702.7290007146585 custom-test\client-request-body.js method="end" len=256 type="utf" dur=5: 2,665.5040332541416 custom-test\client-request-body.js method="end" len=1024 type="utf" dur=5: 2,586.063423082629 custom-test\client-request-body.js method="end" len=32 type="buf" dur=5: 2,634.4734375277553 custom-test\client-request-body.js method="end" len=256 type="buf" dur=5: 2,646.007782080513 custom-test\client-request-body.js method="end" len=1024 type="buf" dur=5: 2,665.0541256502593

url.parse()

custom-test\client-request-body.js method="end" len=32 type="asc" dur=5: 2,735.6415161268537 custom-test\client-request-body.js method="end" len=256 type="asc" dur=5: 2,718.3595932607045 custom-test\client-request-body.js method="end" len=1024 type="asc" dur=5: 2,696.7241118731454 custom-test\client-request-body.js method="end" len=32 type="utf" dur=5: 2,698.5838802790495 custom-test\client-request-body.js method="end" len=256 type="utf" dur=5: 2,641.479793821612 custom-test\client-request-body.js method="end" len=1024 type="utf" dur=5: 2,608.204761809595 custom-test\client-request-body.js method="end" len=32 type="buf" dur=5: 2,506.5034363033974 custom-test\client-request-body.js method="end" len=256 type="buf" dur=5: 2,597.553156245891 custom-test\client-request-body.js method="end" len=1024 type="buf" dur=5: 2,652.0725942522863

jasnell · 2018-04-26T16:36:13Z

@Hackzzila ... thank you for this. I think I'm leaning more in favor of the alternative approach that @TimothyGu uses here #20288. Specifically, we should allow for fallback to the older url.parse() for the time being when new URL() fails. There will be significantly less chance of breaking user code. Once the deprecation that #20288 adds lands in Node.js 11, we would be free to end-of-life the url.parse() support in Node.js 12. Makes the runway slightly longer but we end up in the same place ultimately.

jasnell

Making it explicit... I'd like to see this and #20288 reconciled before either lands.

BridgeAR · 2018-04-26T16:49:31Z

I personally think it would have been great to incorporate the changes from @TimothyGu instead of having a separate PR.

mcollina

LGTM.

TimothyGu · 2018-04-27T15:40:11Z

This needs tests for three types of cases:

Default to WHATWG URL parser in http.request (and friends) #19468
http://xn--1ug.example/ where a deprecation message is shown
Completely invalid URLs where an ERR_INVALID_URL is now thrown

TimothyGu · 2018-04-27T23:08:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14556/

This switches the url parser from `url.parse()` to the WHATWG URL parser while keeping `url.parse()` as fallback. Also add tests for invalid url deprecations and correct hostname checks. PR-URL: nodejs#20270Fixes: nodejs#19468 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BridgeAR · 2018-04-29T15:04:05Z

Landed in 564048d 🎉

@Hackzzila thanks a lot for sticking to it!

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

http: replace url.parse() with WHATWG URL parser
4d78329
Fixes: nodejs#19468

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Apr 25, 2018

Hackzzila mentioned this pull request Apr 25, 2018
Default to WHATWG URL parser in http.request (and friends) #19468
Closed

devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 25, 2018

vsemozhetbyt reviewed Apr 25, 2018
View reviewed changes

vsemozhetbyt added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 25, 2018

doc: fix order of links
7e8d81f

vsemozhetbyt added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 25, 2018

lpinca approved these changes Apr 25, 2018
View reviewed changes

TimothyGu mentioned this pull request Apr 25, 2018
WIP: http: replace url.parse with WHATWG URL parser #20288
Closed
4 tasks

jasnell requested changes Apr 26, 2018
View reviewed changes

Hackzzila changed the title ~~http: replace url.parse() with WHATWG URL parser~~tls,http,https: replace url.parse() with WHATWG URL parserApr 26, 2018

mcollina approved these changes Apr 27, 2018
View reviewed changes

test: add tests for invalid url deprecations and correct hostname checks
3b20da1

TimothyGu approved these changes Apr 27, 2018
View reviewed changes

jasnell approved these changes Apr 28, 2018
View reviewed changes

watilde approved these changes Apr 28, 2018
View reviewed changes

trivikr approved these changes Apr 28, 2018
View reviewed changes

BridgeAR approved these changes Apr 29, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2018

BridgeAR closed this Apr 29, 2018

TimothyGu mentioned this pull request Apr 30, 2018
http silently corrupts the request URL when it contains non-Latin-1 codepoints #13296
Closed

ChALkeR mentioned this pull request Jun 25, 2018
doc: remove 2 unused error codes from errors.md #21491
Merged
4 tasks

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2018

jasnell mentioned this pull request Oct 2, 2018
11.0.0 proposal #23230
Closed
4 tasks

Uh oh!

tls,http,https: replace url.parse() with WHATWG URL parser#20270

tls,http,https: replace url.parse() with WHATWG URL parser #20270

Uh oh!

Conversation

Hackzzila commented Apr 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

devsnek commented Apr 25, 2018

Uh oh!

vsemozhetbytApr 25, 2018

Choose a reason for hiding this comment

Uh oh!

HackzzilaApr 25, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytApr 25, 2018

Choose a reason for hiding this comment

Uh oh!

HackzzilaApr 25, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 25, 2018

Uh oh!

lpinca commented Apr 25, 2018

Uh oh!

Hackzzila commented Apr 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Apr 25, 2018

Uh oh!

mscdex commented Apr 25, 2018

Uh oh!

joyeecheung commented Apr 25, 2018

Uh oh!

BridgeAR commented Apr 25, 2018

Uh oh!

TimothyGu commented Apr 25, 2018

Uh oh!

Hackzzila commented Apr 25, 2018

Uh oh!

TimothyGu commented Apr 25, 2018

Uh oh!

lpinca commented Apr 25, 2018

Uh oh!

Hackzzila commented Apr 25, 2018

Uh oh!

jasnell commented Apr 26, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 26, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Apr 27, 2018

Uh oh!

TimothyGu commented Apr 27, 2018

Uh oh!

BridgeAR commented Apr 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

Hackzzila commented Apr 25, 2018•
edited
Loading

Hackzzila commented Apr 25, 2018•
edited
Loading