tls,http,https: replace url.parse() with WHATWG URL parser by Hackzzila · Pull Request #20270 · nodejs/node

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#20270 Fixes: nodejs#19468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 parser Apr 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!

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!

vsemozhetbyt Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Hackzzila Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Hackzzila Apr 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