perf: cri client memory leak #29988
Merged
perf: cri client memory leak #29988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CDPConnection class instantiates a persistent event emitter, and creates a CDP Client instance via connect(). When this client emits a disconnect event, the CDPConnection will automatically attempt to reconnect, if the connection was not intentionally closed and the connection is created with the reconnect flag. It only listens to 'event' and 'disconnect' events itself, making it easy to unsubscribe from events in the event of a disconnection, helping ease mental load when considering potential memory leaks. Because it uses its own persistent event emitter, event listeners added to the CDPConnection do not need to be re-added when the client reconnects. The CriClient itself no longer handles reconnection logic: it only reacts to connection state changes in order to re-send failed commands and enablements when the CDPConnection restores the CDPClient.
cacieprins
commented
Aug 8, 2024
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
cacie/perf/cri-client-memory-leak
|
| Run status |
|
| Run duration | 53m 22s |
| Commit |
|
| Committer | Cacie Prins |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
5
|
Pending
|
1328
|
Skipped
|
0
|
Passing
|
29312
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
43.26%
|
|
|---|---|
|
Untested elements
|
218
|
|
Tested elements
|
170
|
Accessibility
91.25%
|
|
|---|---|
|
Failed rules
|
5 critical
10 serious
2 moderate
2 minor
|
|
Failed elements
|
945
|
…t', which does not trigger for them
…ation test for service worker bindingCalled
…d integration test for service worker bindingCalled" This reverts commit 4855120.
cli/CHANGELOG.md
Outdated
| @@ -3,6 +3,10 @@ | |||
|
|
|||
| _Released 8/13/2024_ | |||
|
|
|||
| **Performance:** | |||
|
|
|||
| - Fixed a potential memory leak when re-connecting to an unintentionally disconnected CDP connection. Fixes [#29744](/~https://github.com/cypress-io/cypress/issues/29744). Addressed in [#29988](/~https://github.com/cypress-io/cypress/pull/29988) | |||
There was a problem hiding this comment.
Do we want to call out that this is a server side leak? Maybe to help avoid confusion with some of the browser-side ones we've got going on.
|
|
||
| inFlightCommand?.deferred.resolve(response) | ||
| } catch (err) { | ||
| debug('error re-enabling %s: ', command, err) | ||
| if (this._isConnectionError(err)) { | ||
| if (CDPDisconnectedError.isCDPDisconnectedError(err)) { | ||
| // below comment is no longer accurate - ephemeral connection is wrapped by |
There was a problem hiding this comment.
If the comment's no longer accurate, should we delete it?
There was a problem hiding this comment.
Thanks for calling attention - noticed a bug in a path that wasn't getting tested because of it. addressed in 1fb6a52
|
|
||
| debug('sent command, received ', { response }) | ||
| enqueued.deferred.resolve(response) | ||
| debug('resolved enqueued promise') | ||
| } catch (e) { | ||
| debug('enqueued command %s failed:', enqueued.command, e) | ||
| if (this._isConnectionError(e)) { | ||
| if (CDPDisconnectedError.isCDPDisconnectedError(e)) { | ||
| // below comment is no longer accurate - ephemeral connection is wrapped by |
ryanthemanuel
approved these changes
Aug 14, 2024
mschile
reviewed
Aug 14, 2024
mschile
approved these changes
Aug 14, 2024
Co-authored-by: Matt Schile <mschile@cypress.io>
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional details
Event listeners added directly to the CDP Client in
CriClientwere never removed, and referenced the CDP client via closure. Because new CDP clients were created every reconnection attempt, this could lead to zombie references, preventing old CDP Clients from being garbage collected.To fix, the number of event listeners that are being added directly to the
CDP.Clientis no longer variable, is limited to 2, and they are removed when aCDP.Clientconnection is closed.To accomplish that:
remote-interfacedirectory in@packages/server.cdp_automation.tswas left in place, due to being similar in function towebkit-automation.ts.CDPConnectionclass. This class is responsible for:CDP.Client. When indicated,CDPConnectionwill create a newCDP.Clientwhen it receives thedisconnectevent.CDP.Client.EventEmitter, so that event listeners do not need to be removed/added between ephemeralCDP.Clients.CDP.Client.send(), so downstream logic can determine if a command needs to be enqueuedCriClientCriClient's event listeners are added toCDPConnectioninstance when the instance is created, rather than when the ephemeralCDP.Clientinstance is created. This simplifies the connection logic flow, at the expense of (slightly) increasing the complexity of theCriClientconstructor. This change reveals a further refactor opportunity: everything necessary to construct theCDPConnectionand initialize its listeners is actually passed in as parameters toCriClient. The next step for this set of files is to extract theCDPConnectioncreation & subscription logic up one level, and pass in theCDPConnectioninstance to theCriClient. This reduces the parameter list of theCriClient, removing unnecessary information sharing (CriClientis only providedCDPConnectionparameters and callbacks in order to instantiate theCDPConnection).Steps to test
There should be no changes in functionality of Cypress, other than a potential reduction in memory leak issues.
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?