http: add debug message for invalid header value #9195

evanlucas · 2016-10-19T21:33:22Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

http

Description of change

This makes it easier to see what header has an invalid value. As discussed on the CTC call today (10/19/2016), this is a way to get which header is causing the throw without introducing a breaking change (changing error messages is considered breaking)

Ref: #9010

mscdex · 2016-10-19T22:33:24Z

Maybe this doesn't/won't matter to most people, but do we know if V8 will also skip the string creation if it notices that debug() is a noop? I mention this because JSON.stringify() isn't the fastest thing in the world compared to other built-in functions or statements.

I think this may be even more of an issue than before because anyone that would hit this code path would presumably have a try-catch to catch the thrown exception and try-catch no longer causes a deopt of the containing function. If try-catch still resulted in a deopt, one could argue the call to JSON.stringify() wouldn't matter much because that perf hit would pale in comparison to a permanent function deopt.

evanlucas · 2016-10-19T22:35:15Z

Maybe it would be better to use something like debug('blah blah %s', name)? Then we won't incur the penalty of formatting unless the NODE_DEBUG environment variable has http?

cjihrig · 2016-10-20T00:20:04Z

Wouldn't %j do a safe stringify (util.format()), and avoid any penalty when debug() is a noop? Of course, %s should work too.

mscdex · 2016-10-20T00:56:20Z

Yeah using one of those two might be better.

evanlucas · 2016-10-20T21:59:33Z

ok updated. I went with %s since we already know that name is a valid http token

mscdex · 2016-10-20T22:17:51Z

You might add double quotes around the %s to help highlight the header name.

mscdex · 2016-10-20T22:57:25Z

LGTM

jasnell · 2016-10-21T15:11:52Z

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

ChALkeR · 2016-10-23T08:50:30Z

What about The trailer content contains invalid characters error? Shouldn't it be also changed?

This makes it easier to see what header has an invalid value.

evanlucas · 2016-10-25T13:23:04Z

Running CI one more time. @jasnell @thefourtheye @cjihrig, I added two more debug statements, PTAL. Thanks!

This makes it easier to see what header has an invalid value. PR-URL: #9195 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2016-10-28T16:20:38Z

Landed in d93ab51

This makes it easier to see what header has an invalid value. PR-URL: #9195 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2016-11-22T07:13:13Z

is this something we want to backport? If so it will require manual backporting for both lts release streams

MylesBorins · 2017-01-23T01:37:10Z

ping @evanlucas

evanlucas · 2017-01-30T15:39:12Z

Sorry for the delay @MylesBorins. I'll submit backport PRs.

This makes it easier to see what header has an invalid value. PR-URL: nodejs#9195 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

This makes it easier to see what header has an invalid value. PR-URL: #9195 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 19, 2016

evanlucas changed the title ~~http: add debug message for invalid header character~~ http: add debug message for invalid header value Oct 19, 2016

jasnell approved these changes Oct 20, 2016

View reviewed changes

evanlucas force-pushed the debugheaderthrow branch from 869fd1a to 2fdb8aa Compare October 20, 2016 21:58

evanlucas force-pushed the debugheaderthrow branch from 2fdb8aa to 18d6716 Compare October 20, 2016 22:19

thefourtheye approved these changes Oct 21, 2016

View reviewed changes

cjihrig approved these changes Oct 21, 2016

View reviewed changes

http: add debug message for invalid header value

6930512

This makes it easier to see what header has an invalid value.

evanlucas force-pushed the debugheaderthrow branch from 18d6716 to 6930512 Compare October 24, 2016 16:29

jasnell approved these changes Oct 25, 2016

View reviewed changes

thefourtheye approved these changes Oct 25, 2016

View reviewed changes

jasnell closed this Oct 28, 2016

evanlucas deleted the debugheaderthrow branch November 2, 2016 12:50

evanlucas mentioned this pull request Nov 2, 2016

http: improve invalid character in header error message #9010

Closed

4 tasks

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added lts-watch-v4.x labels Nov 22, 2016

evanlucas mentioned this pull request Jan 30, 2017

http: add debug message for invalid header value #11073

Closed

3 tasks

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Mar 3, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

Uh oh!

http: add debug message for invalid header value #9195

http: add debug message for invalid header value #9195

Uh oh!

Conversation

evanlucas commented Oct 19, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlucas commented Oct 19, 2016

Uh oh!

cjihrig commented Oct 20, 2016

Uh oh!

mscdex commented Oct 20, 2016

Uh oh!

evanlucas commented Oct 20, 2016

Uh oh!

mscdex commented Oct 20, 2016

Uh oh!

mscdex commented Oct 20, 2016

Uh oh!

jasnell commented Oct 21, 2016

Uh oh!

ChALkeR commented Oct 23, 2016

Uh oh!

evanlucas commented Oct 25, 2016

Uh oh!

jasnell commented Oct 28, 2016

Uh oh!

MylesBorins commented Nov 22, 2016

Uh oh!

MylesBorins commented Jan 23, 2017

Uh oh!

evanlucas commented Jan 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mscdex commented Oct 19, 2016 •

edited

Loading