querystring: fix empty pairs handling#11234

mscdex · 2017-02-08T08:23:50Z

This is a follow up to #10967, which is (currently) a semver-major.

This PR fixes outstanding issues stemming from the aforementioned PR and includes the tests from #11171. FWIW the original issue for all of this was that empty query string key/value pairs before the end of the string were not being ignored like an empty key/value at the end of the string would be.

I've also included some improvements to the parser in general, along with a couple of new benchmarks to highlight some of those improvements. Here are the results for benchmark/querystring/querystring-parse.js:

 improvement confidence p.value querystring/querystring-parse.js n=1000000 type="altspaces" 84.35 % *** 1.191823e-35 querystring/querystring-parse.js n=1000000 type="encodefake" 2.96 % * 4.610320e-02 querystring/querystring-parse.js n=1000000 type="encodelast" 4.78 % ** 7.673085e-03 querystring/querystring-parse.js n=1000000 type="encodemany" 2.83 % 5.099655e-02 querystring/querystring-parse.js n=1000000 type="manyblankpairs" 192.66 % *** 4.461978e-48 querystring/querystring-parse.js n=1000000 type="manypairs" -0.14 % 9.307657e-01 querystring/querystring-parse.js n=1000000 type="multicharsep" 1.65 % 2.795571e-01 querystring/querystring-parse.js n=1000000 type="multivalue" 1.40 % 3.885362e-01 querystring/querystring-parse.js n=1000000 type="multivaluemany" 1.82 % 4.116370e-01 querystring/querystring-parse.js n=1000000 type="noencode" -0.45 % 8.223732e-01

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

Checklist

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

Affected core subsystem(s)

querystring

This commit fixes handling of empty pairs that occur before the end of the query string so that they are also ignored. Additionally, some optimizations have been made, including: * Avoid unnecessary code execution where possible * Use a lookup table when checking for hex characters * Avoid forced decoding when '+' characters are encountered and we are using the default decoder Fixes: nodejs#10454

mscdex · 2017-02-08T08:43:45Z

lib/querystring.js

+encodeCheck=1;
+continue;
+}elseif(encodeCheck>0){
+// eslint-disable-next-line no-extra-boolean-cast


If anyone is curious, this was added because there seems to be a positive difference when explicitly converting to a boolean in the conditional vs. implicitly converting the numeric value (or undefined) to a boolean. eslint doesn't know that isHexTable contains non-booleans, so I explicitly silence these "errors."

Isn't it better to use isHexTable[code] > 0 as a more readable option? The performance seems to be quite similar

That's debatable. I think the explicit conversion is easier to reason about than remembering how undefined values compare with numeric values.

I agree, in this case I'd propose isHexTable[code] === 1 as an alternative

If anyone is curious, this was added because there seems to be a positive difference when explicitly converting to a boolean in the conditional vs. implicitly converting the numeric value (or undefined) to a boolean.
Could you clarify what you mean by the "positive difference"?

Could you clarify what you mean by the "positive difference"?
An increase in performance?

That's interesting. AFAIK if (foo) has exactly the same behavior as if (!!foo), so I'm not sure why the performance would be different.
If there really is a difference, then I propose adding a comment explaining that.

joyeecheung · 2017-02-08T10:02:40Z

benchmark/querystring/querystring-parse.js

+// timed loop. Instead, just execute the function a "sufficient" number of
+// times before the timed loop to ensure the function is optimized just once.
+if(type==='multicharsep'){
+for(i=0;i<n;i+=1)


Does this need to be n? Refs: v8/src/runtime-profiler.cc, this might change but 1e3 should probabaly be safe.
// Number of times a function has to be seen on the stack before it is// compiled for baseline.staticconstintkProfilerTicksBeforeBaseline = 1; // Number of times a function has to be seen on the stack before it is// optimized.staticconstintkProfilerTicksBeforeOptimization = 2; // If the function optimization was disabled due to high deoptimization count,// but the function is hot and has been seen on the stack this number of times,// then we try to reenable optimization for this function.staticconstintkProfilerTicksBeforeReenablingOptimization = 250; // If a function does not have enough type info (according to// FLAG_type_info_threshold), but has seen a huge number of ticks,// optimize it as it is.staticconstintkTicksWhenNotEnoughTypeInfo = 100; // We only have one byte to store the number of ticks.STATIC_ASSERT(kProfilerTicksBeforeOptimization < 256); STATIC_ASSERT(kProfilerTicksBeforeReenablingOptimization < 256); STATIC_ASSERT(kTicksWhenNotEnoughTypeInfo < 256);

Originally I used n / 2, but I seemed to get better (less variable) results with n, especially considering the default value of n. Either way, nshould be enough no matter what, so I kept it.

stevenvachon · 2017-02-09T23:24:22Z

Please merge asap.

mscdex · 2017-02-09T23:54:32Z

@stevenvachon Other @nodejs/collaborators need to sign off on this first.

stevenvachon · 2017-02-13T16:59:21Z

Hate to nag, but how about now? I'd really like to play with this in a nightly build.

This commit fixes handling of empty pairs that occur before the end of the query string so that they are also ignored. Additionally, some optimizations have been made, including: * Avoid unnecessary code execution where possible * Use a lookup table when checking for hex characters * Avoid forced decoding when '+' characters are encountered and we are using the default decoder Fixes: #10454 PR-URL: #11234 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nicu Micleușanu <[email protected]>

PR-URL: #11234 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nicu Micleușanu <[email protected]>

jasnell · 2017-02-13T18:50:26Z

Landed in ff785fd...8bcc122

This specific bug was fixed by #11234. This commit turns on the tests accordingly. PR-URL: #11369 Ref: #11234 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

stevenvachon · 2017-02-28T01:23:27Z

This has a "dont-land-on-v7.x" label, yet it appears to be working in v7.6

mscdex · 2017-02-28T01:36:22Z

@stevenvachon This didn't land in v7.x. Can you explain what you mean by "working?" For example, querystring.parse('a&&b') still returns {a: '', '': '', b: '' } in v7.6.0, however master returns {a: '', b: '' } (due to this PR being merged in master).

I should also point out that querystring.parse('a&b&') has always resulted in {a: '', b: '' }. This PR was for fixing the case when an empty pair does not occur at the end of the string.

stevenvachon · 2017-02-28T02:12:16Z

This issue was referenced in #11171 as the solution. My comment there, containing a test case that was failing on 7.5.x is now passing on 7.6

mscdex · 2017-02-28T02:37:31Z

@stevenvachon With the test case you're referencing I get the same result on node v0.10.41, v4.0.0, v6.5.0, v7.5.0, v7.6.0, and master:

> querystring.parse('a=&a=value&a='){a: [ '', 'value', '' ] }

stevenvachon · 2017-02-28T04:00:55Z

Shit.. I dunno what's going on then. My original issue is not reproducible with 7.5, which was available days before posting.

mscdex · 2017-02-28T04:21:07Z

@stevenvachon Ah ok, so that was originally using WHATWG URL, which does its parsing in a specific way and its implementation has changed often since node v7.0.0. FWIW though I tried node v7.4.0, v7.5.0, and v7.6.0 and got the same expected result you showed in your original issue, namely:

> const url = new (require("url").URL)("http://www.domain.com:123/dir/file.html?var=&var=value&var=#hash"); undefined > console.log( Array.from(url.searchParams) ); [ [ 'var', '' ], [ 'var', 'value' ], [ 'var', '' ] ] undefined

stevenvachon · 2017-02-28T14:24:30Z

Thank you. Sorry for wasting space in here. I'll have to update a number of test suites as I'd built around this supposed issue.

mscdex added dont-land-on-v4.x querystring Issues and PRs related to the built-in querystring module. labels Feb 8, 2017

mscdex mentioned this pull request Feb 8, 2017
querystring: fix state machine in querystring.parse #11171
Closed
3 tasks

mscdex added 2 commits February 8, 2017 03:40

test: improve querystring.parse assertion messages
7b72507

mscdex force-pushed the querystring-fix-empty-pairs-handling branch from 71973b3 to 7b72507Compare February 8, 2017 08:41

mscdex commented Feb 8, 2017
View reviewed changes

joyeecheung reviewed Feb 8, 2017
View reviewed changes

joyeecheung mentioned this pull request Feb 9, 2017
benchmark: unify input to url-related benchmarks #11264
Closed
3 tasks

jasnell approved these changes Feb 10, 2017
View reviewed changes

micnic approved these changes Feb 10, 2017
View reviewed changes

jasnell pushed a commit that referenced this pull request Feb 13, 2017
test: improve querystring.parse assertion messages
8bcc122
PR-URL: #11234 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nicu Micleușanu <[email protected]>

jasnell closed this Feb 13, 2017

mscdex deleted the querystring-fix-empty-pairs-handling branch February 14, 2017 01:14

TimothyGu added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 14, 2017

TimothyGu mentioned this pull request Feb 14, 2017
url: track status of a spec-compliant URLSearchParams #10821
Closed
11 tasks

joyeecheung mentioned this pull request Feb 14, 2017
test, url: turn on WPT tests on empty params pairs #11369
Closed
3 tasks

jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed

targos mentioned this pull request Jun 19, 2017
querystring.parse decodes incorrect in a specific case #13773
Closed

Uh oh!

querystring: fix empty pairs handling#11234

querystring: fix empty pairs handling #11234

Uh oh!

Conversation

mscdex commented Feb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdexFeb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micnicFeb 10, 2017

Choose a reason for hiding this comment

Uh oh!

mscdexFeb 10, 2017

Choose a reason for hiding this comment

Uh oh!

micnicFeb 10, 2017

Choose a reason for hiding this comment

Uh oh!

seishunFeb 10, 2017

Choose a reason for hiding this comment

Uh oh!

mscdexFeb 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seishunFeb 10, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheungFeb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexFeb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenvachon commented Feb 9, 2017

Uh oh!

mscdex commented Feb 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenvachon commented Feb 13, 2017

Uh oh!

jasnell commented Feb 13, 2017

Uh oh!

stevenvachon commented Feb 28, 2017

Uh oh!

mscdex commented Feb 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenvachon commented Feb 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Feb 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenvachon commented Feb 28, 2017

Uh oh!

mscdex commented Feb 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenvachon commented Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mscdex commented Feb 8, 2017•
edited
Loading

mscdexFeb 8, 2017•
edited
Loading

mscdexFeb 10, 2017•
edited
Loading

joyeecheungFeb 8, 2017•
edited
Loading

mscdexFeb 8, 2017•
edited
Loading

mscdex commented Feb 9, 2017•
edited
Loading

mscdex commented Feb 28, 2017•
edited
Loading

stevenvachon commented Feb 28, 2017•
edited
Loading

mscdex commented Feb 28, 2017•
edited
Loading

mscdex commented Feb 28, 2017•
edited
Loading