Proposal: Add an API to recover after DISCONNECT

[nicojs]

Thanks again for the awesome work on Karma. There might already be an API for what I'm asking, but I couldn't find it. I hope I'm not wasting your time.

Current behavior

When running StrykerJS with karma (via @stryker-mutator/karma-runner), the code under test is being changed and can have unexpected behavior. One of the possibilities is that an infinite loop is created. For example:

while (result) {
-  this.x++;
+  this.x--;
  if (this.x == 10) {
    result = false;
  }
}

This will result in the browser being really busy and not able to ping to the server, eventually causing a "Disconnected reconnect failed before timeout of 2000ms (ping timeout)" error.

There currently is no way to recover from this. The browser isn't restarted or closed and there is no way to force this in the karma API. The only solution is to close the entire node process and start a new one, which is very expensive (especially in scenario's where webpack needs to rerun).

Desired solution

Add a way to recover from this scenario. For example:

const karmaConfig = { /*...*/ };
const server = karma.Server(karmaConfig);
server.once('browsers_ready', () => {
    server.once('browser_error', () => {
       // new proposed API here:
       await karma.launcher.close(karmaConfig);
       await karma.launcher.start(karmaConfig);
    });
    karma.runner.run(karmaConfig);
});

Additional question: would the solution in #3267 change this disconnect behavior?

Related issue
#3267
stryker-mutator/stryker-js#2989

[nicojs]

Hmm I've found a reference to restart in the typescript API: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/karma/v4/index.d.ts#L45

I'm now trying to implement it like this:

function recoverDisconnect(launcher, server) {
        server.on('browser_error', (browser) => {
            if(browser.state === 'EXECUTING_DISCONNECTED'){
                console.log('restarting!')
                launcher.restart(browser.id);
        }
    });
}

recoverDisconnect.$inject = ['launcher', 'server'];

async function main() {
    const config = await karma.config.parseConfig(require.resolve('./karma.conf.js'), { frameworks: ['recoverDisconnect',  'chai', 'mocha'], plugins: [ 'karma-*', { 'framework:recoverDisconnect': ['factory', recoverDisconnect] },], singleRun: false }, { promiseConfig: true, throwErrors: true });

    const server = new karma.Server(config);
    server .on('run_complete', async (_browser, results) => {
        console.log(results);
    });

    server .once('browsers_ready', async () => {
        karma.runner.run(config);
    });
    await s.start();
}

main().catch(err => {
    console.error(err);
    process.exitCode = 1;
});

However, restart seems to call childProcess.kill internally, which in turn makes it so that the main process exits, see:

karma/lib/launchers/process.js

Lines 82 to 84 in 2b71a3c

	self._process.on('exit', function (code, signal) {
	self._onProcessExit(code, signal, errorOutput)
	})

This seems by design. What is the usecase for restart?

[nicojs]

Aha, I found what the issue was. I was indeed doing something wrong 😅

Providing callbacks to karma's methods is really important. I didn't provide a callback to karma.runner.run. If you forget to provide a callback, it defaults to process.exit.

    server .once('browsers_ready', async () => {
-        karma.runner.run(config);
+        karma.runner.run(config, () => {});
    });

If I provide a callback it seems to work as expected and I'm able to restart the browser as expected 🤟

Only question that remains is: is this a stable API? I guess time will tell... 😅?

[devoto13]

Depending on what you're trying to achieve, maybe http://karma-runner.github.io/6.3/config/configuration-file.html#browserdisconnecttolerance will work?

Only question that remains is: is this a stable API? I guess time will tell... 😅?

He-he, indeed! It's rather hard to give any guarantees on this as Karma does not have a formal public API and almost any change may become breaking to somebody.

Additional question: would the solution in #3267 change this disconnect behavior?

I think the error message will change somewhat, but otherwise, the logic will stay the same as before: if the browser didn't send ping until ping timeout, it will be considered disconnected and will fire the browser_error event.

UPD Oh, the EXECUTING_DISCONNECTED state will be gone :(

Providing callbacks to karma's methods is really important. I didn't provide a callback to karma.runner.run. If you forget to provide a callback, it defaults to process.exit.

Yeah.. We've already removed a bunch of process.exit() calls from the codebase. I hope to change this to noop at some point as well.

[devoto13]

There currently is no way to recover from this. The browser isn't restarted or closed and there is no way to force this in the karma API. The only solution is to close the entire node process and start a new one, which is very expensive (especially in scenario's where webpack needs to rerun).

I think we should kill the browser process upon disconnect in the #3653 implementation. I'll look into it when I've got some time.

[nicojs]

UPD Oh, the EXECUTING_DISCONNECTED state will be gone :(

Hmm, my current implementation is this:

  if (browser.state.toUpperCase().includes('DISCONNECTED')) {
    // Restart the browser for next run
    this.karmaServer.get('launcher').restart(browser.id);
    this.browserIsRestarting = true;
  }

So any state with 'DISCONNECTED' in the name will work. Is that future proof-ish? 😅

https://github.com/stryker-mutator/stryker-js/blob/20e3d260a27175ccfe97f6eba4cfc58b795ec8a9/packages/karma-runner/src/karma-plugins/stryker-reporter.ts#L130-L134

I think we should kill the browser process upon disconnect in the #3653 implementation.

Would there still be a possibility to start a new one automatically? Or will that be the default behavior?

If you want I can validate any changes you make to karma before release. We have a pretty robust test set for integrating StrykerJS with Karma, including infinite loops, multiple runs, exiting the process, angular integration, windows/linux, a karma-webpack scenario, etc. That would also improve the migration experience for Stryker users to the latest karma release.

[devoto13]

Is that future proof-ish? 😅

Yes, I would say so!

Would there still be a possibility to start a new one automatically? Or will that be the default behavior?

Yes, there would be a possibility to start a new one. We actually already kill the disconnected browser. So the only real difference is that browser_error will be emitted immediately without waiting for the browser to potentially reconnect after the ping timeout.

If you want I can validate any changes you make to karma before release.

Thanks! I'll ping you once that PR is ready for testing.

[hans-lizihan]

any updates on this issue? Hit the same issue here WRT the socket disconnection.

[nicojs]

We've implemented our disconnect fix and it's working ever since: https://github.com/stryker-mutator/stryker-js/blob/master/packages/karma-runner/src/karma-plugins/stryker-reporter.ts#L135-L151