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

fix: AST rewriting when inline script is very long #8048

Merged
merged 8 commits into from
Jul 23, 2020
Merged

Conversation

panzarino
Copy link
Contributor

@panzarino panzarino commented Jul 21, 2020

User facing changelog

Fixes an issue where AST rewriting would return an output before the body was done being written. This would happen when the response body was too large and the response would be sent while the body was still being modified.

Additional details

This following test case would fail with the previous implementation, but now passes: https://github.com/cypress-io/cypress/pull/8048/files#diff-2aeeae49664314a9a544ffdea59ec137R55.

From the issue, here's an example of a body getting cut off:

Screen Shot 2020-07-20 at 6 32 38 PM

Here's what the response looks like now:

Screen Shot 2020-07-21 at 5 19 15 PM

Note: Thanks to Murphy's law, the test case from this issue still does not pass (due to something unrelated).

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 21, 2020

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jul 21, 2020



Test summary

7902 0 130 2


Run details

Project cypress
Status Passed
Commit 7f8c58f
Started Jul 23, 2020 5:14 PM
Ended Jul 23, 2020 5:21 PM
Duration 06:52 💡
OS Linux Debian - 10.1
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@panzarino panzarino marked this pull request as ready for review July 21, 2020 21:28
@panzarino panzarino requested a review from flotwig July 21, 2020 21:28
@@ -58,7 +58,7 @@ parentPort!.on('message', (req: RewriteRequest) => {
}

try {
const output = _getOutput()
const output = await _getOutput()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panzarino i thought this wasn't working like we expected, did you figure it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was working, turns out the issue I was seeing was from somewhere else and it was completely unrelated. I also tested the async functions specifically and it was working fine.

flotwig
flotwig previously approved these changes Jul 22, 2020
@panzarino panzarino changed the title Fix AST rewriting when inline script is very long Fix: AST rewriting when inline script is very long Jul 23, 2020
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. i'm still confused as to why/when the chunk splitting happens tho.

@flotwig flotwig changed the title Fix: AST rewriting when inline script is very long fix: AST rewriting when inline script is very long Jul 23, 2020
@panzarino
Copy link
Contributor Author

panzarino commented Jul 23, 2020

@flotwig it's split by the RewritingStream. Each chunk is the result of the response in html-rules.ts: for example when it sends a rewriter.emitRaw(raw), that's a chunk.

@flotwig
Copy link
Contributor

flotwig commented Jul 23, 2020

@panzarino That doesn't explain why this only happens after a certain body size, since the other test cases emit as one chunk.

@panzarino
Copy link
Contributor Author

@flotwig Wasn't aware of that, not sure why its not working like that this time then.

Should I be concerned with the failing server performance test?

@flotwig
Copy link
Contributor

flotwig commented Jul 23, 2020

I think that's flake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AST rewriting not adding closing script tag when string is too long
2 participants