Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop capturing panics and trying to continue execution after them. #3748

Closed
mstoykov opened this issue May 20, 2024 · 0 comments · Fixed by #3777
Closed

Stop capturing panics and trying to continue execution after them. #3748

mstoykov opened this issue May 20, 2024 · 0 comments · Fixed by #3777
Assignees
Milestone

Comments

@mstoykov
Copy link
Contributor

mstoykov commented May 20, 2024

What?

Drop the following code and it uses:

k6/js/common/util.go

Lines 67 to 87 in 153e0e5

// RunWithPanicCatching catches panic and converts into an InterruptError error that should abort a script
func RunWithPanicCatching(logger logrus.FieldLogger, rt *goja.Runtime, fn func() error) (err error) {
defer func() {
if r := recover(); r != nil {
gojaStack := rt.CaptureCallStack(20, nil)
err = &errext.InterruptError{Reason: fmt.Sprintf("a panic occurred during JS execution: %s", r)}
// TODO figure out how to use PanicLevel without panicing .. this might require changing
// the logger we use see
// https://github.com/sirupsen/logrus/issues/1028
// https://github.com/sirupsen/logrus/issues/993
b := new(bytes.Buffer)
for _, s := range gojaStack {
s.Write(b)
}
logger.Error("panic: ", r, "\n", string(debug.Stack()), "\nGoja stack:\n", b.String())
}
}()
return fn()
}

Why?

The code was added many years ago when k6 had a bunch of really hard to fix race conditions. And was left as a safeguard.

Unfortunately it has two problems - it is not enough for most cases with multiple goroutines/async. And probably even worse - makes a bunch of potential bugs seem like less of an issue.

We have had at least one user come and ask us to have this be optional as it was problem during extension development.

Also, as far as I am aware it hasn't caught a single issue where k6 continuing has been a good idea for years. Some of the early things were nice, but now it seems like it just gets in the way.

Suggestion:

Just remove the code and it's called site to be rewritten with no defers.

@mstoykov mstoykov added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label May 20, 2024
@mstoykov mstoykov added this to the v0.52.0 milestone May 20, 2024
@mstoykov mstoykov removed the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Jun 5, 2024
@olegbespalov olegbespalov self-assigned this Jun 6, 2024
@olegbespalov olegbespalov mentioned this issue Jun 6, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants