fix: Time out unmatched prerequests in proxy to avoid leaking memory #22462
Conversation
…(and generally improve proxy performance)
|
Thanks for taking the time to open a PR!
|
|
Could this have any impact on |
This prerequest/request pair in this context is a CDP event "The browser says it's about to make a request" and "the server received a request from the browser". It's a tight internal loop, to map these two events together. Usually takes a millisecond or two, and it'd only be slowed down my an overloaded CPU, not by user-supplied timeouts / cypress commands. |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
(Removed TIm as reviewer, it added him automatically when I accidentally included some GQL files I hadn't intended to touch) |
|
Just wanted to drop in and say this refactor is great work! |
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
…o/cypress into issue-17853-prerequest-timeout
…esser/CLOUD-577-spec-list-display-latest-runs-batching * muaz/CLOUD-577-spec-list-display-latest-runs: fix: Update "Request Access" button state after requesting access (ACI) (#22499) feat: Support "Queued" latest run status (#22497) fix: remove ctx.cloud.reset in tests, handle infinite loop in stale results (#22483) chore: add reporter webpack to gulp watch script (#22386) fix: Increase timeout for npm-webpack-dev-server tests (#22489) fix: Time out unmatched prerequests in proxy to avoid leaking memory (#22462) fix: Sort results in findCrossOriginLogs test helper to deterministic (#22481) fix: memory leak caused by storing base64 encoded files recieved by CDP `Network.requestWillBeSent` (#22460) fix: Improve cross-origin cookie handling (#22320) feat: Add button to clear value from search fields (#22202) chore: Add test to verify settings panels are collapsed by default (#22382) fix: process_profiler follow up work for v10 (#22363) chore: Update Chrome (stable) to 103.0.5060.53 (#22441) refactor: use design system windicss config (#21503) chore: update readme logo (#22433) chore: Update Chrome (beta) to 103.0.5060.53 (#22351) chore: updating version (#22432) Trigger Build
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
No user facing impact.
Additional details
Refactored the prerequest proxy logic to be easier to follow. It now uses plain javascript objects with a string key to identify prerequests and requests, and creates fewer closure functions.
It also sweeps unmatched prerequests more than 10s old, and doesn't retain references to requests in the
metricsobject, ensuring that we're not leaking memory. I have not verified that this occurs in scenarios that #22460 doesn't already address, but I consider the refactor and minor performance gain to be worthwhile even with that PR making it in.This should be considered a companion to, but not replacement for #22460. That PR has better performance in the case of data urls by catching them much sooner. This refactor and update is aimed more at readability, and ensuring we don't leak memory in other situations (which may or may not be happening).
Steps to test
This PR does not require manual testing. Our proxy logic is extensively tested with e2e and unit tests.
Run the unit tests for the
proxypackage. Existing tests are ~1% faster according to mocha, which is hard to notice because they're already quite fast. (this PR does add a couple of additional tests covering existing and new functionality, so total test time is longer than before on my machine).How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?