-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Unhandled "WebSocket connection closed" when CDP connection is unstable #29830
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
71a93aa
unit and integration tests that reproduce websocket disconnected unha…
cacieprins 42be544
WIP: command queue
cacieprins d2fc2d1
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins dd0da67
complete command queue and retry refactor
cacieprins e637979
cri-client changes pass tests; modify certain tests for readability a…
cacieprins a6cb8a6
removes unnecessary logic from command queue, adds unit tests for com…
cacieprins 927902a
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins b24d6a0
rm unused cdp state - this should be reserved for future refactor
cacieprins 8dde990
small edits to cri-client: better error handling, more comprehensive …
cacieprins b763ddb
comment re: queue property
cacieprins f2450fb
rearrange cri client member methods for readability
cacieprins 78e82a4
further edits
cacieprins 0b19fe9
Changelog
cacieprins 673d86b
Update cli/CHANGELOG.md
cacieprins ac82a83
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins 422fe2f
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
jennifer-shehane 1e3440e
fix continuous retry on close
cacieprins 98fae10
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
jennifer-shehane bf06572
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins 4c5d899
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins fc52033
split heavier debugs to verbose
cacieprins 6a4031b
Merge branch 'develop' into cacie/fix/ws-disconnected-trampoline
cacieprins File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import type ProtocolMapping from 'devtools-protocol/types/protocol-mapping' | ||
import pDefer, { DeferredPromise } from 'p-defer' | ||
import type { CdpCommand } from './cdp_automation' | ||
import Debug from 'debug' | ||
|
||
const debug = Debug('cypress:server:browsers:cdp-command-queue') | ||
const debugVerbose = Debug('cypress:server:browsers:cd-command-queue') | ||
|
||
type CommandReturn<T extends CdpCommand> = ProtocolMapping.Commands[T]['returnType'] | ||
|
||
export type Command<T extends CdpCommand> = { | ||
command: T | ||
params?: object | ||
deferred: DeferredPromise<CommandReturn<T>> | ||
sessionId?: string | ||
} | ||
|
||
export class CDPCommandQueue { | ||
private queue: Command<any>[] = [] | ||
|
||
public get entries () { | ||
return [...this.queue] | ||
} | ||
|
||
public add <TCmd extends CdpCommand> ( | ||
command: TCmd, | ||
params: ProtocolMapping.Commands[TCmd]['paramsType'][0], | ||
sessionId?: string, | ||
): Promise<CommandReturn<TCmd>> { | ||
debug('enqueing command %s', command) | ||
debugVerbose('enqueing command %s with params %o', command, params) | ||
|
||
const deferred = pDefer<CommandReturn<TCmd>>() | ||
|
||
const commandPackage: Command<TCmd> = { | ||
command, | ||
params, | ||
deferred, | ||
sessionId, | ||
} | ||
|
||
this.queue.push(commandPackage) | ||
|
||
debug('Command enqueued; new length: %d', this.queue.length) | ||
debugVerbose('Queue Contents: %O', this.queue) | ||
|
||
return deferred.promise | ||
} | ||
|
||
public clear () { | ||
debug('clearing command queue') | ||
this.queue = [] | ||
} | ||
|
||
public extract<T extends CdpCommand> (search: Partial<Command<T>>): Command<T> | undefined { | ||
// this should find, remove, and return if found a given command | ||
|
||
const index = this.queue.findIndex((enqueued) => { | ||
for (let k of Object.keys(search)) { | ||
if (search[k] !== enqueued[k]) { | ||
return false | ||
} | ||
} | ||
|
||
return true | ||
}) | ||
|
||
debug('extracting %o from commands at index %d', search, index) | ||
|
||
if (index === -1) { | ||
return undefined | ||
} | ||
|
||
const [extracted] = this.queue.splice(index, 1) | ||
|
||
return extracted | ||
} | ||
|
||
public shift () { | ||
return this.queue.shift() | ||
} | ||
|
||
public unshift (value: Command<any>) { | ||
return this.queue.unshift(value) | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How chatty will these logs be? Do we want to make some of them verbose and some of them non-verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very chatty - commands are only pushed into the command queue when they come in while the websocket connection is unexpectedly disconnected. params might be a larger payload, though, and could benefit from a verbose (or removal, as it's not pertinent to the command queue operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in fc52033