Always pass NODE_OPTIONS with max-http-header-size #5452

flotwig · 2019-10-24T16:58:45Z

Reverted in #5522

Originally meant for: 431 status code on stubbed XHR requests with large response in 3.5.0 #5431

User facing changelog

Fixes a regression in 3.5.0 where the maximum size of an HTTP header or cy.route() stub body was limited to 8kb and requests would fail with HTTP error 431.

Additional details

Passes --max-http-header-size=${1024 * 1024} in NODE_OPTIONS environment var from CLI
- Passing it as a CLI arg (the way we thought it worked) does not seem to work with vanilla Electron binary or with built Cypress binary, so this is the only way
- By ordering it before the user-supplied NODE_OPTIONS, the user can still override this via NODE_OPTIONS if they choose
Makes 4_xhr_spec e2e test go through CLI so it can run a test with this max http header size set
User's original NODE_OPTIONS are backed up and restored once Electron successfully launches so that node processes launched by Cypress on behalf of the user have desired NODE_OPTIONS

How has the user experience changed?

N/A

PR Tasks

Have tests been added/updated?

cypress-bot · 2019-10-24T16:58:47Z

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

The feature/bugfix is self-documenting from within the product.
The change provides the end user with a way to fix their problem (no dead ends).

Functionality

The code works and performs its intended function with the correct logic.
Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

The code is readable (too many nested 'if's are a bad sign).
Names used for variables, methods, etc, clearly describe their function.
The code is easy to understood and there are relevant comments explaining.
New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
There are comments containing link(s) to the addressed issue (in tests and code).

Quality

The change does not reimplement code.
There's not a module from the ecosystem that should be used instead.
There is no redundant or duplicate code.
There are no irrelevant comments left in the code.
Tests are testing the code’s intended functionality in the best way possible.

Internal

The original issue has been tagged with a release in ZenHub.

flotwig · 2019-10-24T19:22:12Z

Hm, Mac kitchensink is failing because it only has support for Node 11. Since it's in dev mode, it's launching node instead of electron and failing because 11 doesn't have this option. Might be better to just figure out all the places where this can be explicitly passed and pass it...

EDIT: Actually, --max-http-header-size doesn't seem to have any impact when passed to the package app. It only works if supplied in NODE_OPTIONS, which is also the only documented way I could find to pass Node CLI options to Node.js.

andrew-codes

It appears like appropriate tests were added and I was able to follow the changes in relation to the PR's functionality. The whitespace changes made it a little hard to discern for a single file, but not impossible. I imagine having those whitespace changes would be ideal given what they were.

jennifer-shehane

I ran this branch in the services repo - where basically all of their tests were failing with this 431 error - they are now passing. I didn't review the code.

jennifer-shehane · 2019-10-25T20:56:39Z

cli/lib/util.js

+    if (options.dev && nodeVersion < 12) {
+      // `node` is used when --dev is passed, so this won't work if Node is too old
+      logger.warn('(dev-mode warning only) NODE_OPTIONS=--max-http-header-size could not be set. See https://github.com/cypress-io/cypress/pull/5452')
+


Will the new max-header log in DEBUG logs for all of our users? Cause I'd like any new options to be logged in their DEBUG logs for future debugging.

It is not logged anywhere :/ But, it should always be passed, as this is the only condition where it wouldn't be.

This reverts commit 978d97e.

* cli: set NODE_OPTIONS=--max-http-header-size=1024*1024 on spawn * electron: remove redundant max-http-header-size * server: add useCli option to make e2e tests go thru cli * server: add test for XHR with body > 100kb via CLI * clean up conditional * cli: don't pass --max-http-header-size in dev w node < 11.10 * add original_node_options to restore o.g. user node_options * force no color

* Detect if NODE_OPTIONS are present in binary; if not, respawn * Always reset NODE_OPTIONS, even if no ORIGINAL_ Co-authored-by: Andrew Smith <andrew@andrew.codes> * Exit with correct code # from stub process * Clean up based on Brian's feedback * how process.versions is null, i have no idea, but it is * add repro for invalid header char * Always pass NODE_OPTIONS with max-http-header-size (#5452) * cli: set NODE_OPTIONS=--max-http-header-size=1024*1024 on spawn * electron: remove redundant max-http-header-size * server: add useCli option to make e2e tests go thru cli * server: add test for XHR with body > 100kb via CLI * clean up conditional * cli: don't pass --max-http-header-size in dev w node < 11.10 * add original_node_options to restore o.g. user node_options * force no color * Revert "Use websockets to stub large XHR response bodies instead of hea… (#5525)" This reverts commit 249db45. * fix yarn.lock * update 4_xhr_spec snapshot * make 6_visit_spec reproduce invalid header char error * pass --http-parser=legacy * still set headers if an ERR_INVALID_CHAR is raised * add --http-parser=legacy in some more places * update http_requests_spec * readd spawn_spec * improve debug logging * remove unnecessary changes * cleanup * revert yarn.lock to develop * use cp.spawn, not cp.fork to work around the Electron patch: https://github.com/electron/electron/blob/39baf6879011c0fe8cc975c7585567c7ed0aeed8/patches/node/refactor_alter_child_process_fork_to_use_execute_script_with.patch Co-authored-by: Andrew Smith <andrew@andrew.codes>

flotwig added 4 commits October 24, 2019 12:50

cli: set NODE_OPTIONS=--max-http-header-size=1024*1024 on spawn

02f5917

electron: remove redundant max-http-header-size

3521065

server: add useCli option to make e2e tests go thru cli

b08ee27

server: add test for XHR with body > 100kb via CLI

35f864c

flotwig added 2 commits October 24, 2019 13:05

Merge remote-tracking branch 'origin/develop' into issue-5431-431-error

96983f3

clean up conditional

33df694

flotwig added 3 commits October 24, 2019 17:36

cli: don't pass --max-http-header-size in dev w node < 11.10

0975407

add original_node_options to restore o.g. user node_options

eab3ddf

force no color

e63a9bc

flotwig changed the title ~~[WIP] Always pass NODE_OPTIONS with max-http-header-size~~ Always pass NODE_OPTIONS with max-http-header-size Oct 24, 2019

Merge branch 'develop' into issue-5431-431-error

5547022

flotwig requested a review from a team October 24, 2019 23:15

jennifer-shehane mentioned this pull request Oct 24, 2019

431 status code on stubbed XHR requests with large response in 3.5.0 #5431

Closed

Merge branch 'develop' into issue-5431-431-error

a14b2b7

andrew-codes self-requested a review October 25, 2019 20:37

andrew-codes approved these changes Oct 25, 2019

View reviewed changes

jennifer-shehane reviewed Oct 25, 2019

View reviewed changes

jennifer-shehane merged commit 978d97e into develop Oct 25, 2019

This was referenced Oct 28, 2019

[WIP] Detect if NODE_OPTIONS are present in binary; if not, respawn #5492

Closed

There is no way to set --max-http-header-size in Electron 5.x electron/electron#20831

Closed

flotwig added a commit that referenced this pull request Oct 29, 2019

Revert "Always pass NODE_OPTIONS with max-http-header-size (#5452)"

ead7aa9

This reverts commit 978d97e.

This was referenced Oct 29, 2019

Revert "Always pass NODE_OPTIONS with max-http-header-size" #5522

Merged

Use websockets to stub large XHR response bodies instead of headers #5525

Merged

flotwig mentioned this pull request Mar 17, 2020

Fix "Parse Error" when performing HTTP requests #5988

Merged

5 tasks

flotwig deleted the issue-5431-431-error branch January 24, 2022 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always pass NODE_OPTIONS with max-http-header-size #5452

Always pass NODE_OPTIONS with max-http-header-size #5452

flotwig commented Oct 24, 2019 •

edited

cypress-bot bot commented Oct 24, 2019

flotwig commented Oct 24, 2019 •

edited

andrew-codes left a comment

jennifer-shehane left a comment

jennifer-shehane Oct 25, 2019

flotwig Oct 25, 2019

Always pass NODE_OPTIONS with max-http-header-size #5452

Always pass NODE_OPTIONS with max-http-header-size #5452

Conversation

flotwig commented Oct 24, 2019 • edited

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Oct 24, 2019

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

flotwig commented Oct 24, 2019 • edited

andrew-codes left a comment

Choose a reason for hiding this comment

jennifer-shehane left a comment

Choose a reason for hiding this comment

jennifer-shehane Oct 25, 2019

Choose a reason for hiding this comment

flotwig Oct 25, 2019

Choose a reason for hiding this comment

flotwig commented Oct 24, 2019 •

edited

flotwig commented Oct 24, 2019 •

edited