Skip to content

Commit

Permalink
feat(server): remove socket reconnect support
Browse files Browse the repository at this point in the history 
This feature has been trouble and there are cases it cannot work.
Any socket disconnect during a test can lose data. The server will disconnect
if the client does not reply. Combined it makes any long running or large
test error prone.  Better to bail and restart the test.
Fixes karma-runner#3652
  • Loading branch information
@johnjbarton
johnjbarton committed Apr 15, 2021
1 parent 94cf15e commit 2e45262
Show file tree
Hide file tree
Showing 11 changed files with 105 additions and 395 deletions.
3 changes: 0 additions & 3 deletions client/karma.js
View file
Expand Up @@ -282,9 +282,6 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
this.complete()
}.bind(this))

// Report the browser name and Id. Note that this event can also fire if the connection has
// been temporarily lost, but the socket reconnected automatically. Read more in the docs:
// https://socket.io/docs/client-api/#Event-%E2%80%98connect%E2%80%99
socket.on('connect', function () {
socket.io.engine.on('upgrade', function () {
resultsBufferLimit = 1
Expand Down
6 changes: 4 additions & 2 deletions client/main.js
View file
Expand Up @@ -12,8 +12,10 @@ var BROWSER_SOCKET_TIMEOUT = constants.BROWSER_SOCKET_TIMEOUT

// Connect to the server using socket.io https://socket.io/
var socket = io(location.host, {
reconnectionDelay: 500,
reconnectionDelayMax: Infinity,
// We can't support reconnection without message replay: any messages sent
// after disconnect are lost.
reconnection: false,
// At this timeout the client disconnects and the server sees 'transport close'
timeout: BROWSER_SOCKET_TIMEOUT,
path: KARMA_PROXY_PATH + KARMA_URL_ROOT.substr(1) + 'socket.io',
'sync disconnect on unload': true
Expand Down
147 changes: 39 additions & 108 deletions lib/browser.js
View file
Expand Up @@ -4,39 +4,36 @@ const BrowserResult = require('./browser_result')
const helper = require('./helper')
const logger = require('./logger')

const CONNECTED = 'CONNECTED' // The browser is connected but not yet been commanded to execute tests.
const CONNECTED = 'CONNECTED' // The browser is connected but not yet been commanded to execute tests, or finished testing.
const CONFIGURING = 'CONFIGURING' // The browser has been told to execute tests; it is configuring before tests execution.
const EXECUTING = 'EXECUTING' // The browser is executing the tests.
const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is executing the tests, but temporarily disconnect (waiting for socket reconnecting).
const DISCONNECTED = 'DISCONNECTED' // The browser got completely disconnected (e.g. browser crash) and can be only restored with a restart of execution.
/* The browser got completely disconnected (e.g. browser crash) and can be only restored with a restart of execution. */
const DISCONNECTED = 'DISCONNECTED'

class Browser {
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay,
noActivityTimeout, singleRun, clientConfig) {
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, pingTimeout, singleRun, clientConfig) {
this.id = id
this.fullName = fullName
this.name = helper.browserFullNameToShort(fullName)
this.lastResult = new BrowserResult()
this.disconnectsCount = 0
this.activeSockets = [socket]
this.noActivityTimeout = noActivityTimeout
this.singleRun = singleRun
this.clientConfig = clientConfig
this.collection = collection
this.emitter = emitter
this.socket = socket
this.timer = timer
this.disconnectDelay = disconnectDelay
// Set onto the socket when it was created.
this.pingTimeout = pingTimeout

this.log = logger.create(this.name)

this.noActivityTimeoutId = null
this.pendingDisconnect = null
this.setState(CONNECTED)
}

init () {
this.log.info(`Connected on socket ${this.socket.id} with id ${this.id}`)
this.log.info(`Binding socket ${this.socket.id} to browser with id ${this.id}`)

this.bindSocketEvents(this.socket)
this.collection.add(this)
Expand All @@ -53,7 +50,6 @@ class Browser {
this.lastResult.error = true
}
this.emitter.emit('browser_error', this, error)
this.refreshNoActivityTimeout()
}

onInfo (info) {
Expand All @@ -70,8 +66,6 @@ class Browser {
} else if (!helper.isDefined(info.dump)) {
this.emitter.emit('browser_info', this, info)
}

this.refreshNoActivityTimeout()
}

onStart (info) {
Expand All @@ -82,78 +76,45 @@ class Browser {
this.lastResult = new BrowserResult(info.total)
this.setState(EXECUTING)
this.emitter.emit('browser_start', this, info)
this.refreshNoActivityTimeout()
}

onComplete (result) {
if (this.isNotConnected()) {
this.setState(CONNECTED)
this.lastResult.totalTimeEnd()

this.emitter.emit('browsers_change', this.collection)
this.emitter.emit('browser_complete', this, result)

this.clearNoActivityTimeout()
if (this.state !== EXECUTING) {
this.log.warn(`Unexpected:'complete' event arrived while in state ${this.state}`)
}
this.lastResult.totalTimeEnd()
this.setState(CONNECTED)
this.socket.disconnect(true)
}

onSocketDisconnect (reason, disconnectedSocket) {
helper.arrayRemove(this.activeSockets, disconnectedSocket)
if (this.activeSockets.length) {
this.log.debug(`Disconnected ${disconnectedSocket.id}, still have ${this.getActiveSocketsIds()}`)
if (this.state === DISCONNECTED) {
return
}

if (this.isConnected()) {
this.disconnect(`Client disconnected from CONNECTED state (${reason})`)
} else if ([CONFIGURING, EXECUTING].includes(this.state)) {
this.log.debug(`Disconnected during run, waiting ${this.disconnectDelay}ms for reconnecting.`)
this.setState(EXECUTING_DISCONNECTED)

this.pendingDisconnect = this.timer.setTimeout(() => {
this.lastResult.totalTimeEnd()
this.lastResult.disconnected = true
this.disconnect(`reconnect failed before timeout of ${this.disconnectDelay}ms (${reason})`)
this.emitter.emit('browser_complete', this)
}, this.disconnectDelay)

this.clearNoActivityTimeout()
}
}

reconnect (newSocket, clientSaysReconnect) {
if (!clientSaysReconnect || this.state === DISCONNECTED) {
this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
this.setState(CONNECTED)

// The disconnected browser is already part of the collection.
// Update the collection view in the UI (header on client.html)
this.emitter.emit('browsers_change', this.collection)
// Notify the launcher
this.emitter.emit('browser_register', this)
// Execute tests if configured to do so.
if (this.singleRun) {
this.execute()
// Done with this socket
disconnectedSocket.disconnect()
disconnectedSocket.removeAllListeners()

if (this.state !== CONNECTED) {
// If we loaded the context or began testing, `disconnect` loses data.
this.emitter.emit('browser_error', this, `Disconnected from state ${this.state} with reason: ${reason || ''}`)
if (reason === 'ping timeout') {
this.log.info('Check for a test that crashes the browser or tab.')
this.log.info(`For large JS or tests that take longer than ${this.pingTimeout}ms, increase 'config.pingTimeout'`)
} else if (reason === 'transport close') {
this.log.info('The client may have disconnected.')
}
} else if (this.state === EXECUTING_DISCONNECTED) {
this.log.debug('Lost socket connection, but browser continued to execute. Reconnected ' +
`on socket ${newSocket.id}.`)
this.setState(EXECUTING)
} else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
this.log.debug(`Rebinding to new socket ${newSocket.id} (already have ` +
`${this.getActiveSocketsIds()})`)
}

if (!this.activeSockets.some((s) => s.id === newSocket.id)) {
this.activeSockets.push(newSocket)
this.bindSocketEvents(newSocket)
}
this.lastResult.totalTimeEnd()
this.lastResult.disconnected = true

if (this.pendingDisconnect) {
this.timer.clearTimeout(this.pendingDisconnect)
this.timer.setTimeout(() => {
// If we disconnect during a test the server can retry the browser
this.disconnectsCount++
this.finishDisconnect()
}, this.disconnectDelay)
} else {
this.finishDisconnect()
}

this.refreshNoActivityTimeout()
}

onResult (result) {
Expand All @@ -163,23 +124,15 @@ class Browser {
this.lastResult.add(result)
this.emitter.emit('spec_complete', this, result)
}
this.refreshNoActivityTimeout()
}

execute () {
this.activeSockets.forEach((socket) => socket.emit('execute', this.clientConfig))
this.socket.emit('execute', this.clientConfig)
this.setState(CONFIGURING)
this.refreshNoActivityTimeout()
}

getActiveSocketsIds () {
return this.activeSockets.map((s) => s.id).join(', ')
}

disconnect (reason) {
this.log.warn(`Disconnected (${this.disconnectsCount} times) ${reason || ''}`)
this.disconnectsCount++
this.emitter.emit('browser_error', this, `Disconnected ${reason || ''}`)
finishDisconnect () {
this.emitter.emit('browser_complete', this)
this.remove()
}

Expand All @@ -188,26 +141,6 @@ class Browser {
this.collection.remove(this)
}

refreshNoActivityTimeout () {
if (this.noActivityTimeout) {
this.clearNoActivityTimeout()

this.noActivityTimeoutId = this.timer.setTimeout(() => {
this.lastResult.totalTimeEnd()
this.lastResult.disconnected = true
this.disconnect(`, because no message in ${this.noActivityTimeout} ms.`)
this.emitter.emit('browser_complete', this)
}, this.noActivityTimeout)
}
}

clearNoActivityTimeout () {
if (this.noActivityTimeout && this.noActivityTimeoutId) {
this.timer.clearTimeout(this.noActivityTimeoutId)
this.noActivityTimeoutId = null
}
}

bindSocketEvents (socket) {
// TODO: check which of these events are actually emitted by socket
socket.on('disconnect', (reason) => this.onSocketDisconnect(reason, socket))
Expand Down Expand Up @@ -246,7 +179,6 @@ class Browser {
state: this.state,
lastResult: this.lastResult,
disconnectsCount: this.disconnectsCount,
noActivityTimeout: this.noActivityTimeout,
disconnectDelay: this.disconnectDelay
}
}
Expand All @@ -255,17 +187,16 @@ class Browser {
Browser.factory = function (
id, fullName, /* capturedBrowsers */ collection, emitter, socket, timer,
/* config.browserDisconnectTimeout */ disconnectDelay,
/* config.browserNoActivityTimeout */ noActivityTimeout,
/* config.pingTimeout */ pingTimeout,
/* config.singleRun */ singleRun,
/* config.client */ clientConfig) {
return new Browser(id, fullName, collection, emitter, socket, timer,
disconnectDelay, noActivityTimeout, singleRun, clientConfig)
disconnectDelay, pingTimeout, singleRun, clientConfig)
}

Browser.STATE_CONNECTED = CONNECTED
Browser.STATE_CONFIGURING = CONFIGURING
Browser.STATE_EXECUTING = EXECUTING
Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED
Browser.STATE_DISCONNECTED = DISCONNECTED

module.exports = Browser
27 changes: 13 additions & 14 deletions lib/server.js
View file
Expand Up @@ -295,20 +295,19 @@ class Server extends KarmaEventEmitter {
const knownBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null

if (knownBrowser) {
knownBrowser.reconnect(socket, info.isSocketReconnect)
} else {
const newBrowser = this._injector.createChild([{
id: ['value', info.id || null],
fullName: ['value', (helper.isDefined(info.displayName) ? info.displayName : info.name)],
socket: ['value', socket]
}]).invoke(Browser.factory)

newBrowser.init()

if (config.singleRun) {
newBrowser.execute()
singleRunBrowsers.add(newBrowser)
}
this.log.debug(`A previously captured browser has connected (${info.id} with reconnect ${info.isSocketReconnect})`)
}
const newBrowser = this._injector.createChild([{
id: ['value', info.id || null],
fullName: ['value', (helper.isDefined(info.displayName) ? info.displayName : info.name)],
socket: ['value', socket]
}]).invoke(Browser.factory)

newBrowser.init()

if (config.singleRun) {
newBrowser.execute()
singleRunBrowsers.add(newBrowser)
}

replySocketEvents()
Expand Down
6 changes: 4 additions & 2 deletions static/karma.js
View file
Expand Up @@ -334,8 +334,10 @@ var BROWSER_SOCKET_TIMEOUT = constants.BROWSER_SOCKET_TIMEOUT

// Connect to the server using socket.io https://socket.io/
var socket = io(location.host, {
reconnectionDelay: 500,
reconnectionDelayMax: Infinity,
// We can't support reconnection without message replay: any messages sent
// after disconnect are lost.
reconnection: false,
// At this timeout the client disconnects and the server sees 'transport close'
timeout: BROWSER_SOCKET_TIMEOUT,
path: KARMA_PROXY_PATH + KARMA_URL_ROOT.substr(1) + 'socket.io',
'sync disconnect on unload': true
Expand Down
27 changes: 0 additions & 27 deletions test/e2e/reconnecting.feature
View file

This file was deleted.

5 changes: 0 additions & 5 deletions test/e2e/support/reconnecting/plus.js
View file

This file was deleted.

34 changes: 0 additions & 34 deletions test/e2e/support/reconnecting/test.js
View file

This file was deleted.

0 comments on commit 2e45262

Please sign in to comment.