Non-iterational goroutines should not abort an iteration

[inancgumus]

Brief summary

Goroutines other than the one that drives an iteration should not panic. Otherwise, k6 won't catch the panic and fail.

We should take control of our non-iteration goroutines: Not panic and cause an iteration to be prematurely aborted.

More details about how to craft a solution:

• Treat go panics as an interrupt error k6#2453
• https://github.com/grafana/k6/blob/d81bb4162517064bdaf9b23bc15062e08184757d/js/runner.go#L788-L793
• https://github.com/grafana/k6/blob/d81bb4162517064bdaf9b23bc15062e08184757d/js/bundle.go#L334-L348

xk6-browser version

v0.7.0

OS

Staging Cloud

Docker version and image (if applicable)

grafana/k6-cloud-scripts-to-archive:2.40.6-xk6browser-v0.7.0-1

Steps to reproduce the problem

Pawel ran the following script, and we got sentry alerts because of the panics.

This script panics.

import { check, sleep } from 'k6';
import { Counter } from 'k6/metrics';
import { chromium } from 'k6/x/browser';


const checkpoint1 = new Counter('checkpoint1');
const checkpoint2 = new Counter('checkpoint2');
const checkpoint3 = new Counter('checkpoint3');
const checkpoint4 = new Counter('checkpoint4');
const checkpoint5 = new Counter('checkpoint5');
const checkpoint6 = new Counter('checkpoint6');
const checkpoint7 = new Counter('checkpoint7');


export let options = {
  vus: 100,
  duration: '10h',
}

export function setup(){
  console.log("Running setup")
}

export default function () {
  const browser = chromium.launch({
    headless: true
  });
  const context = browser.newContext();
  const page = context.newPage();

  checkpoint1.add(1);

  // Goto front page, find login link and click it
  page.goto('https://test.k6.io/', { waitUntil: 'networkidle' });

  checkpoint2.add(1);

  Promise.all([
    page.waitForNavigation(),
    page.locator('a[href="/my_messages.php"]').click(),
  ]).then(() => {
    checkpoint3.add(1)

    // Enter login credentials and login
    page.locator('input[name="login"]').type('admin');
    page.locator('input[name="password"]').type('123');
    // We expect the form submission to trigger a navigation, so to prevent a
    // race condition, setup a waiter concurrently while waiting for the click
    // to resolve.

    checkpoint4.add(1)

    return Promise.all([
      page.waitForNavigation(),
      page.locator('input[type="submit"]').click(),
    ]);
  }).then(() => {
    checkpoint5.add(1)

    check(page, {
      'header': page.locator('h2').textContent() == 'Welcome, admin!',
    });
  }).finally(() => {
    checkpoint6.add(1)
    page.close();
    browser.close();
    checkpoint7.add(1)
  });
  sleep(1);
}

export function teardown() {
  console.log("Running teardown")
}

The script has an incorrect usage of promises, and the fix is easy:

page.goto('https://test.k6.io/', { waitUntil: 'networkidle' });

// this should be page.goto(...).then(...)

However, the problem is deeper than it seems.

See more details in the test run: 1620122 (proxy).

Actual behaviour

Panic causes an iteration to be prematurely aborted.

Tasks

Beta Give feedback

1. Don't panic in Response
2. Don't panic in JSHandle
3. Don't panic in ElementHandle
4. Don't panic in Frame
5. Don't panic in Page
6. Don't panic in Worker
7. Don't panic in BrowserContext
8. Don't panic in Browser
9. Don't panic in BrowserType
10. TBA: Look at the remaining panics and split them up again into smaller tasks.
11. Move k6ext.Panic into the mapping layer.
12. Move k6ext.Promise into the mapping layer and incorporate panicIfInternalError in it.

[ankur22]

Types of panic

1. ✅ go panic. -- all documented
2. k6ext.Panic.
3. ✅ k6common.Throw. -- all documented

Panic vs Error

Error

Should denote to the user that an error occurred but there's a way around it, i.e. they can fix it themselves. This shouldn't stop the whole test but just the current iteration.

Panic

Should denote an abort of the whole test run (all iterations) as an unexpected internal error was encountered which we need to fix e.g. the goja runtime is missing from the content. The assumption i'm making is that if it happens in the current iteration it's probably happening in all iterations.

Areas where I think are ok to use panic (abort the whole test run)

NOTE:

Where	runs on main thread	Reason
xk6-browser/tests/test_browser.go Line 349 in 63d5db7 panic("too many arguments to promiseThen")	Y	used in unit/integration tests
xk6-browser/tests/test_browser.go Line 92 in 63d5db7 panic(fmt.Errorf("testBrowser: unexpected browser type %T", v))	Y	used in unit/integration tests
xk6-browser/k6ext/panic.go Line 34 in 16c0b8c defer k6common.Throw(rt, fmt.Errorf(format, a...))	Y	k6ext.panic eventually uses a k6common.Throw, this seems fine for now as long as k6ext.Panic is used sensibly
xk6-browser/log/logger.go Line 181 in 9047580 panic(fmt.Sprintf("internal error while getting goroutine ID: %v", err))	Y	panics when goroutine id can't be found during logging
xk6-browser/keyboardlayout/layout.go Line 72 in 4485c6d panic(fmt.Sprintf("keyboard layout already registered: %s", lang))	Y	When keyboard registration fails
xk6-browser/k6ext/panic.go Line 39 in 16c0b8c panic("no browser process ID in context")	Y	No browser PID
xk6-browser/k6ext/panic.go Line 22 in 16c0b8c panic("no k6 JS runtime in context")	Y	No k6 goja runtime
xk6-browser/browser/module.go Line 56 in f9153fd k6common.Throw(vu.Runtime(), errors.New(msg))	Y	To prevent cloud runs
xk6-browser/browser/mapping.go Line 52 in 575b975 k6common.Throw(rt, fmt.Errorf("mapping: %w", err))	Y	When the goja mapping fails
xk6-browser/common/options.go Line 50 in 9047580 panic("dst should be one of: map, struct, slice")	Y	programmer error if we're not passing in the correct type (change to k6ext.Panic)
xk6-browser/common/options.go Line 41 in 9047580 panic("dst should be a pointer")	Y	programmer error if we're not passing in the correct type (change to k6ext.Panic)
xk6-browser/common/frame.go Line 401 in 575b975 panic(err)	N	programmer error if we're not passing in the correct type (change to k6ext.Panic)
xk6-browser/common/frame_session.go Line 991 in 9884e91 panic(err)	Y	Programmer error -- trying to update subframe viewport which can only be done on the main frame (change to k6ext.Panic)
xk6-browser/common/element_handle.go Line 1508 in 575b975 panic(fmt.Errorf("unexpected DOM error type %T", v))	?	unexpected error that we now need to code in
xk6-browser/common/element_handle.go Line 1504 in 575b975 panic("DOM error is nil")	?	nil error from dom, programmer needs to investigate symptom

List of panics that could be errors (only stop the current test iteration)

Where	reason
xk6-browser/chromium/browser_type.go Line 68 in e62e7c1 k6common.Throw(rt, errors.New("BrowserType.connect() has not been implemented yet"))	unimplemented method, user can remove the call
xk6-browser/chromium/browser_type.go Line 210 in e62e7c1 k6common.Throw(rt, errors.New("BrowserType.LaunchPersistentContext(userDataDir, opts) has not been implemented yet"))	unimplemented method, user can remove the call

Question

1. Can we return an error to goja instead of panicking?
   1. Yes we can. I tested this with the Launch method, and the behaviour seems to be the same if we panic from Launch or return an error. The difference is that the API changes from Launch(opts goja.Value) api.Browser to Launch(opts goja.Value) (api.Browser, error). POC: Refactor the Launch code to work with errors #718
   2. IMO a panic should be used to denote an abort of the whole test run (all iterations) as an unexpected internal error was encountered which we need to fix e.g. the goja runtime is missing from the content.
   3. IMO an error should denote a user error that can be fixed by the user themselves e.g. the element with name "foo" couldn't be found, and a time out occured. This shouldn't stop the whole test but just the current iteration.
2. Panic in code is ok if running on the "main" thread, which should mean it's ok to panic from APIs, e.g. goto since it will only stop the current iteration.
   1. This is true. A panic on the "main" thread will stop the current iteration but not the whole test run.

[ankur22]

We briefly discussed the strategy going forward, and we've agreed that we should pick a file and convert the errors into panic panics into errors where appropriate and ensure that a panic is only used if we need to abort the whole test process due to an unexpected internal error.

Based on the feedback from the first PR, we should be able to convert the rest of the codebase.

[inancgumus]

@ankur22, what I have in mind is to let the mapping layer handle the panics and the internal (core/business logic) parts of the code return errors. This is more idiomatic and maintainable in the long run (from the perspective of tests + code). But it's not something you necessarily need to tackle in this issue. I agree with the rest of your findings 👍

[ankur22]

That's a good idea. We could assert on the behaviour of the error to determine whether to panic or return the error: https://dave.cheney.net/2014/12/24/inspecting-errors