Skip to content

Commit

Permalink
Fixes #25
Browse files Browse the repository at this point in the history
There is a race condition between test.stop() and native runner
monitor.

Our stop implementation ensures that after a DELETE call, the status
returned for a given job does say 'stopped', which happens with retries.
The native runner monior is sneaking through this period to see twice
that status is stopped but native runner has not marked it as stopped
yet.

The solution used here is that as soon as the test results arrive and
we find a valid non-stopped test to stop, a state 'nativeRunnerStopping'
is added to the test. When native runner monitor sees this state, it
just won't process this test any more to figure out if it has been not
reported yet etc. So for actually failing tests that do not respond back,
the earlier logic (now encapsulated under the if condition checking for
this state) comes into play, and would determine correctly, free of the
race conditions, that the test did not stop correctly.
  • Loading branch information
reeteshranjan committed Oct 11, 2017
1 parent b7a09be commit 1f0a865
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 8 deletions.
3 changes: 3 additions & 0 deletions bin/server/runners/native/run/manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,16 +161,19 @@ class Manager {
}

function stopTest(test, takeScreenshot) {
test.nativeRunnerStopping = true
handleScreenshot(test, takeScreenshot)
.then(() => {
return test.stop()
})
.then(() => {
test.nativeRunnerStopped = true
delete test.nativeRunnerStopping
})
.catch(err => {
log.warn('encountered error while stopping test %s', err)
test.nativeRunnerStopped = true
delete test.nativeRunnerStopping
})
}

Expand Down
18 changes: 10 additions & 8 deletions bin/server/runners/native/run/monitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,16 @@ class Monitor {
if('JS' !== test.nativeRunnerConfig.type) {
completedTests.push(test)
}
else if(!test.nativeRunnerConfig.sawStopped) {
log.debug('test %s has been seen stopped once without being marked by native runner', test.serverId)
test.nativeRunnerConfig.sawStopped = true
}
else {
delete test.nativeRunnerConfig.sawStopped
this._processJsTestRetries(test)
completedTests.push(test)
else if(!test.nativeRunnerStopping) {
if(!test.nativeRunnerConfig.sawStopped) {
log.debug('test %s has been seen stopped once without being marked by native runner', test.serverId)
test.nativeRunnerConfig.sawStopped = true
}
else {
delete test.nativeRunnerConfig.sawStopped
this._processJsTestRetries(test)
completedTests.push(test)
}
}
}
})
Expand Down

0 comments on commit 1f0a865

Please sign in to comment.