fix(proxy/prerequests): fix duplicate key behavior, fallthrough

[flotwig]

• Closes

User facing changelog

• Fixed an issue introduced in 10.3.0 where network logs could be missing certain information or cause certain requests to be delayed by 500ms.

Additional details

• Performance fixes in fix: Time out unmatched prerequests in proxy to avoid leaking memory #22462 caused a couple of unintended changes:
  • Missing return statement on line 77 here causes a matched prerequest to be queued anyways:

    cypress/packages/proxy/lib/http/util/prerequests.ts

    Lines 69 to 83 in 8fb785a

    	addPending (browserPreRequest: BrowserPreRequest) {
    	metrics.browserPreRequestsReceived++
    	const key = `${browserPreRequest.method}-${browserPreRequest.url}`
    	if (this.pendingRequests[key]) {
    	debugVerbose('Incoming pre-request %s matches pending request. %o', key, browserPreRequest)
    	clearTimeout(this.pendingRequests[key].timeout)
    	this.pendingRequests[key].callback(browserPreRequest)
    	delete this.pendingRequests[key]
    	}
    	debugVerbose('Caching pre-request %s to be matched later. %o', key, browserPreRequest)
    	this.pendingPreRequests[key] = browserPreRequest
    	this.prerequestTimestamps[key] = Date.now()
    	}

  • More impactful, the Record<string, ...> data structure would clobber any duplicate url+method requests/pre-requests, which could manifest itself in incorrect pre-request data or unintended delays of requests by 500ms.
    • Replaced this with a StackMap which supports duplicate keys and .pop by key.

Steps to test

How has the user experience changed?

PR Tasks

• Have tests been added/updated?
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[flotwig]

You can run this spec and replace this fn with:

  function expectPendingCounts (pendingRequests: number, pendingPreRequests: number) {
    expect(Object.entries(preRequests.pendingRequests).length).to.eq(pendingRequests, 'wrong number of pending requests')
    expect(Object.entries(preRequests.pendingPreRequests).length).to.eq(pendingPreRequests, 'wrong number of pending prerequests')
  }

to see the previous behavior, all 4 tests failed on this assertion with a leak.

[cypress]

---

Test summary

37846 • 0 • 469 • 0 • 4

---

Run details

Project	cypress
Status	Passed
Commit	ba215e0
Started	Aug 10, 2022 3:52 PM
Ended	Aug 10, 2022 4:13 PM
Duration	21:07 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	cypress/proxy-logging.cy.ts		2
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
2	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
	e2e/origin/commands/assertions.cy.ts		1
1	cy.origin assertions > #consoleProps > .should() and .and()
	next.cy.ts		1
1	Working with next-12.1.6 > should detect new spec

---

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

[ryanthemanuel]

I initially left a comment, but I had misunderstood what was going on. I'm pretty clear now. Looks good!

[cypress-bot]

Released in 10.5.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.5.0, please open a new issue.