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

flotwig · 2019-10-29T21:27:10Z

User facing changelog

Fixed a regression where cy.route stubs with a response of more than about 8 kilobytes of data would cause an HTTP 431 error to occur. Fixes #5431 #5542 #5541

Additional details

Temporary workaround for Always pass NODE_OPTIONS with max-http-header-size #5452 [WIP] Detect if NODE_OPTIONS are present in binary; if not, respawn #5492
Most users get the 431 when they are stubbing a large response XHR body
This is because the XHR response is sent to the backend (for stubbing the actual response body) as a header X-Cypress-Response
This PR makes it so that for responses > 4kb, they will be serialized, sent over the websocket, and then fulfilled with the body asynchronously on the server-side when the request is received
4kb was chosen because it is less than the new maximum header size (8kb), and it leaves extra room for things like big Cookie headers and Authorization headers

How has the user experience changed?

Large response stubs work.

PR Tasks

Have tests been added/updated?

cypress-bot · 2019-10-29T21:27:12Z

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.

…nc payload

packages/server/lib/xhr_ws_server.ts

brian-mann · 2019-10-30T05:35:50Z

packages/server/lib/xhr_ws_server.ts

+    debug('resetting incomingXhrs %o', { length: incomingXhrs.length })
+
+    _.forEach(incomingXhrs, ({ reject }) => {
+      reject(new Error('This stubbed XHR was pending on a stub response object from the driver, but the test ended before that happened.'))


this does not feel correct - tests can end while the XHR is in flight without that being an actual error

This is really just to ensure that the HTTP request is closed before creating new tests; otherwise, the connections would pile up until they timed out.

It could also be fulfilled with a successful response, or left to time out. Ed: Or we could try killing the socket.

packages/server/lib/socket.coffee

packages/runner/src/lib/event-manager.js

jennifer-shehane · 2019-10-30T15:47:56Z

Pulling this down to run against Services to see if it fixes their requests.

jennifer-shehane

~~This is creating a new failure within the Services team - when running their tests in this branch.~~

~~Essentially the test is something like:~~

cy.server()
cy.fixture('runs').as('runs')
/// later
cy.route('runs?page=1', this.runs).as('getRuns')

~~The runs.json file being quite large: https://github.com/cypress-io/cypress-services/blob/feature/cypress-3.5.0/packages/dashboard/cypress/fixtures/runs.json~~

Just kidding, I commented something out in the JSON 😅 rerunning.

jennifer-shehane

This fixes the tests that were previously failing in the Services repo. I did not review the code.

Dismiss due to user error :P

packages/driver/src/cy/commands/xhr.coffee

packages/server/lib/xhr_ws_server.ts

packages/server/lib/controllers/xhrs.coffee

…ea… (#5525)" This reverts commit 249db45.

* 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 2 commits October 29, 2019 17:39

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

743339e

Use websockets to stub large XHR response bodies instead of headers

e8f6e88

flotwig force-pushed the xhr-ws branch from d425fdb to e8f6e88 Compare October 29, 2019 21:39

Merge branch 'develop' into xhr-ws

21dd3e9

flotwig mentioned this pull request Oct 29, 2019

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

Closed

3 tasks

flotwig changed the title ~~[WIP] Use websockets to stub large XHR response bodies instead of headers~~ Use websockets to stub large XHR response bodies instead of headers Oct 29, 2019

flotwig requested a review from brian-mann October 29, 2019 22:01

flotwig added 7 commits October 29, 2019 18:15

Properly cleanup outstanding XHRs on before:test:run

d03cb5f

Merge branch 'xhr-ws' of github.com:cypress-io/cypress into xhr-ws

1078351

Add lil unit test for xhr_ws_server

17b4ad3

Merge branch 'develop' into xhr-ws

a7ad0db

Use reset:xhr:server to get around sending entire test:before:run:asy…

f5180a3

…nc payload

Merge branch 'xhr-ws' of github.com:cypress-io/cypress into xhr-ws

520def1

Merge branch 'develop' into xhr-ws

5cf5837

flotwig commented Oct 30, 2019

View reviewed changes

packages/server/lib/xhr_ws_server.ts Outdated Show resolved Hide resolved

brian-mann reviewed Oct 30, 2019

View reviewed changes

brian-mann requested changes Oct 30, 2019

View reviewed changes

packages/server/lib/socket.coffee Outdated Show resolved Hide resolved

packages/server/lib/socket.coffee Outdated Show resolved Hide resolved

packages/runner/src/lib/event-manager.js Outdated Show resolved Hide resolved

flotwig added 3 commits October 30, 2019 10:56

Responding to feedback

58278c5

Merge remote-tracking branch 'origin/develop' into xhr-ws

21c4587

Merge branch 'xhr-ws' of github.com:cypress-io/cypress into xhr-ws

6db262b

flotwig requested a review from brian-mann October 30, 2019 15:24

jennifer-shehane self-requested a review October 30, 2019 15:47

jennifer-shehane previously requested changes Oct 30, 2019

View reviewed changes

jennifer-shehane self-requested a review October 30, 2019 16:05

jennifer-shehane reviewed Oct 30, 2019

View reviewed changes

jennifer-shehane mentioned this pull request Oct 30, 2019

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

Closed

brian-mann requested changes Oct 30, 2019

View reviewed changes

packages/driver/src/cy/commands/xhr.coffee Outdated Show resolved Hide resolved

packages/server/lib/xhr_ws_server.ts Outdated Show resolved Hide resolved

packages/server/lib/xhr_ws_server.ts Outdated Show resolved Hide resolved

Implement feedback

e697c45

flotwig requested a review from brian-mann October 30, 2019 18:31

brian-mann requested changes Oct 30, 2019

View reviewed changes

packages/server/lib/controllers/xhrs.coffee Show resolved Hide resolved

move data obj wrapping into xhrs controller

07dee28

brian-mann approved these changes Oct 30, 2019

View reviewed changes

brian-mann merged commit 249db45 into develop Oct 30, 2019

flotwig added a commit that referenced this pull request Mar 12, 2020

Revert "Use websockets to stub large XHR response bodies instead of h…

d77c106

…ea… (#5525)" This reverts commit 249db45.

flotwig mentioned this pull request Mar 17, 2020

Fix "Parse Error" when performing HTTP requests #5988

Merged

5 tasks

flotwig deleted the xhr-ws branch January 24, 2022 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

flotwig commented Oct 29, 2019 •

edited by brian-mann

cypress-bot bot commented Oct 29, 2019

brian-mann Oct 30, 2019

flotwig Oct 30, 2019 •

edited

jennifer-shehane commented Oct 30, 2019

jennifer-shehane left a comment •

edited

jennifer-shehane left a comment

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

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

Conversation

flotwig commented Oct 29, 2019 • edited by brian-mann

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Oct 29, 2019

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

brian-mann Oct 30, 2019

Choose a reason for hiding this comment

flotwig Oct 30, 2019 • edited

Choose a reason for hiding this comment

jennifer-shehane commented Oct 30, 2019

jennifer-shehane left a comment • edited

Choose a reason for hiding this comment

jennifer-shehane left a comment

Choose a reason for hiding this comment

flotwig commented Oct 29, 2019 •

edited by brian-mann

flotwig Oct 30, 2019 •

edited

jennifer-shehane left a comment •

edited