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

http: add debug message for invalid header value #9195

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 19, 2016
@evanlucas evanlucas changed the title http: add debug message for invalid header character http: add debug message for invalid header value Oct 19, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 19, 2016

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
Copy link
Contributor Author

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
Copy link
Contributor

cjihrig commented Oct 20, 2016

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
Copy link
Contributor

mscdex commented Oct 20, 2016

Yeah using one of those two might be better.

@evanlucas
Copy link
Contributor Author

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

@mscdex
Copy link
Contributor

mscdex commented Oct 20, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Oct 20, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 21, 2016

@ChALkeR
Copy link
Member

ChALkeR commented Oct 23, 2016

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
Copy link
Contributor Author

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

jasnell pushed a commit that referenced this pull request Oct 28, 2016
This makes it easier to see what header has an invalid value.

PR-URL: #9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2016

Landed in d93ab51

@jasnell jasnell closed this Oct 28, 2016
@evanlucas evanlucas deleted the debugheaderthrow branch November 2, 2016 12:50
evanlucas added a commit that referenced this pull request Nov 3, 2016
This makes it easier to see what header has an invalid value.

PR-URL: #9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

ping @evanlucas

@evanlucas
Copy link
Contributor Author

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

evanlucas added a commit to evanlucas/node that referenced this pull request Feb 7, 2017
This makes it easier to see what header has an invalid value.

PR-URL: nodejs#9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2017
This makes it easier to see what header has an invalid value.

PR-URL: #9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2017
This makes it easier to see what header has an invalid value.

PR-URL: #9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
This makes it easier to see what header has an invalid value.

PR-URL: #9195
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
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
MylesBorins added a commit that referenced this pull request Mar 21, 2017
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    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 <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    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 <[email protected]>
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.

8 participants