Skip to content

Commit

Permalink
fix: preserve browser cri client in election if orphan process is det…
Browse files Browse the repository at this point in the history
…ected [run ci]
  • Loading branch information
AtofStryker committed May 14, 2024
1 parent 4d80ef8 commit da1febb
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 9 deletions.
17 changes: 12 additions & 5 deletions packages/server/lib/browsers/electron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const _getAutomation = async function (win, options: BrowserLaunchOpts, parent)
const port = getRemoteDebuggingPort()

if (!browserCriClient) {
debug(`browser CRI is not set. Creating...`)
browserCriClient = await BrowserCriClient.create({
hosts: ['127.0.0.1'],
port,
Expand All @@ -68,6 +69,7 @@ const _getAutomation = async function (win, options: BrowserLaunchOpts, parent)
const pageCriClient = await browserCriClient.attachToTargetUrl('about:blank')

const sendClose = async () => {
debug(`sendClose called, browserCriClient is set? ${!!browserCriClient}`)
if (browserCriClient) {
const gracefulShutdown = true

Expand Down Expand Up @@ -443,10 +445,15 @@ export = {
* Clear instance state for the electron instance, this is normally called on kill or on exit, for electron there isn't any state to clear.
*/
clearInstanceState (options: GracefulShutdownOptions = {}) {
debug('closing remote interface client', { options })
// Do nothing on failure here since we're shutting down anyway
browserCriClient?.close(options.gracefulShutdown).catch(() => {})
browserCriClient = null
// in the case of orphaned browser clients, we should preserve the CRI client as it is connected
// to the same host regardless of window
debug('clearInstanceState called with options', { options })
if (!options.shouldPreserveCriClient) {
debug('closing remote interface client')
// Do nothing on failure here since we're shutting down anyway
browserCriClient?.close(options.gracefulShutdown).catch(() => {})
browserCriClient = null
}
},

connectToNewSpec (browser: Browser, options: ElectronOpts, automation: Automation) {
Expand Down Expand Up @@ -534,7 +541,7 @@ export = {
allPids: [mainPid],
browserWindow: win,
kill (this: BrowserInstance) {
clearInstanceState({ gracefulShutdown: true })
clearInstanceState({ gracefulShutdown: true, shouldPreserveCriClient: this.isOrphanedBrowserProcess })

if (this.isProcessExit) {
// if the process is exiting, all BrowserWindows will be destroyed anyways
Expand Down
13 changes: 9 additions & 4 deletions packages/server/lib/browsers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ interface KillOptions {
isProcessExit?: boolean
nullOut?: boolean
unbind?: boolean
isOrphanedBrowserProcess?: boolean
}

const kill = (options: KillOptions = {}) => {
options = _.defaults({}, options, {
instance,
isProcessExit: false,
isOrphanedBrowserProcess: false,
unbind: true,
nullOut: true,
})
Expand Down Expand Up @@ -60,7 +62,7 @@ const kill = (options: KillOptions = {}) => {
debug('killing browser process')

instanceToKill.isProcessExit = options.isProcessExit

instanceToKill.isOrphanedBrowserProcess = options.isOrphanedBrowserProcess
instanceToKill.kill()
})
}
Expand Down Expand Up @@ -180,7 +182,7 @@ export = {

const _instance = await browserLauncher.open(browser, options.url, options, automation, ctx.coreData.servers.cdpSocketServer)

debug('browser opened')
debug(`browser opened for launch ${thisLaunchAttempt}`)

// in most cases, we'll kill any running browser instance before launching
// a new one when we call `await kill()` early in this function.
Expand All @@ -204,8 +206,11 @@ export = {
// this browser, it means it has been orphaned and should be terminated.
//
// https://github.com/cypress-io/cypress/issues/24377
if (thisLaunchAttempt !== launchAttempt) {
await kill({ instance: _instance, nullOut: false })
const isOrphanedBrowserProcess = thisLaunchAttempt !== launchAttempt

if (isOrphanedBrowserProcess) {
debug(`killing process because launch attempt: ${thisLaunchAttempt} does not match current launch attempt: ${launchAttempt}`)
await kill({ instance: _instance, isOrphanedBrowserProcess, nullOut: false })

return null
}
Expand Down
2 changes: 2 additions & 0 deletions packages/server/lib/browsers/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export type BrowserInstance = EventEmitter & {
* TODO: remove need for this
*/
isProcessExit?: boolean
isOrphanedBrowserProcess?: boolean
}

export type BrowserLauncher = {
Expand All @@ -52,4 +53,5 @@ export type BrowserLauncher = {

export type GracefulShutdownOptions = {
gracefulShutdown?: boolean
shouldPreserveCriClient?: boolean
}
2 changes: 2 additions & 0 deletions packages/server/test/unit/browsers/browsers_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ describe('lib/browsers/index', () => {
browsers._setInstance(null)

expect(browserInstance1.kill).to.be.calledOnce
expect(browserInstance1.isOrphanedBrowserProcess).to.be.true
expect(currentInstance).to.equal(browserInstance2)
})

Expand Down Expand Up @@ -262,6 +263,7 @@ describe('lib/browsers/index', () => {
browsers._setInstance(null)

expect(browserInstance1.kill).to.be.calledOnce
expect(browserInstance1.isOrphanedBrowserProcess).to.be.true
expect(currentInstance).to.equal(browserInstance2)
})
})
Expand Down
27 changes: 27 additions & 0 deletions packages/server/test/unit/browsers/electron_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,33 @@ describe('lib/browsers/electron', () => {
})
})

context('.kill', () => {
beforeEach(async function () {
await electron._getAutomation({}, { onError: () => {} }, {})

await this.stubForOpen()

sinon.stub(electron, '_getBrowserCriClient').returns(this.browserCriClient)
})

it('does not terminate the browserCriClient if the instance is an orphaned process', async function () {
const instance = await electron.open('electron', this.url, this.options, this.automation)

instance.isOrphanedBrowserProcess = true
instance.kill()

expect(this.browserCriClient.close).not.to.be.called
})

it('terminates the browserCriClient otherwise', async function () {
const instance = await electron.open('electron', this.url, this.options, this.automation)

instance.kill()

expect(this.browserCriClient.close).to.be.called
})
})

context('._launch', () => {
beforeEach(() => {
sinon.stub(menu, 'set')
Expand Down

0 comments on commit da1febb

Please sign in to comment.