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 catching panics #3777

Merged
merged 1 commit into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 0 additions & 101 deletions cmd/panic_integration_test.go

This file was deleted.

38 changes: 0 additions & 38 deletions cmd/root_test.go

This file was deleted.

12 changes: 5 additions & 7 deletions js/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func (b *Bundle) instantiate(vuImpl *moduleVUImpl, vuID uint64) (*sobek.Object,
}()

// TODO: make something cleaner for interrupting scripts, and more unified
// (e.g. as a part of the event loop or RunWithPanicCatching()?
// (e.g. as a part of the event loop?
initDone := make(chan struct{})
go func() {
select {
Expand All @@ -302,12 +302,10 @@ func (b *Bundle) instantiate(vuImpl *moduleVUImpl, vuID uint64) (*sobek.Object,
}()

var exportsV sobek.Value
err = common.RunWithPanicCatching(b.preInitState.Logger, rt, func() error {
return vuImpl.eventLoop.Start(func() error {
var err error
exportsV, err = modSys.RunSourceData(b.sourceData)
return err
})
err = vuImpl.eventLoop.Start(func() error {
var err error
exportsV, err = modSys.RunSourceData(b.sourceData)
return err
})

<-initDone
Expand Down
19 changes: 0 additions & 19 deletions js/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,8 @@ import (
"bytes"
"fmt"
"io"
"runtime/debug"

"github.com/grafana/sobek"
"github.com/sirupsen/logrus"
"go.k6.io/k6/errext"
)

// Throw a JS error; avoids re-wrapping GoErrors.
Expand Down Expand Up @@ -64,22 +61,6 @@ func ToString(data interface{}) (string, error) {
}
}

// RunWithPanicCatching catches panic and converts into an InterruptError error that should abort a script
func RunWithPanicCatching(logger logrus.FieldLogger, _ *sobek.Runtime, fn func() error) (err error) {
defer func() {
if r := recover(); r != 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
logger.Error("panic: ", r, "\n", string(debug.Stack()))
}
}()

return fn()
}

// IsNullish checks if the given value is nullish, i.e. nil, undefined or null.
func IsNullish(v sobek.Value) bool {
return v == nil || sobek.IsUndefined(v) || sobek.IsNull(v)
Expand Down
8 changes: 3 additions & 5 deletions js/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -840,11 +840,9 @@ func (u *VU) runFn(
if u.moduleVUImpl.eventLoop == nil {
u.moduleVUImpl.eventLoop = eventloop.New(u.moduleVUImpl)
}
err = common.RunWithPanicCatching(u.state.Logger, u.Runtime, func() error {
return u.moduleVUImpl.eventLoop.Start(func() (err error) {
v, err = fn(sobek.Undefined(), args...) // Actually run the JS script
return err
})
err = u.moduleVUImpl.eventLoop.Start(func() (err error) {
v, err = fn(sobek.Undefined(), args...) // Actually run the JS script
return err
})

select {
Expand Down
73 changes: 0 additions & 73 deletions js/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2230,79 +2230,6 @@ func TestSystemTags(t *testing.T) {
}
}

func TestVUPanic(t *testing.T) {
t.Parallel()
r1, err := getSimpleRunner(t, "/script.js", `
var group = require("k6").group;
exports.default = function() {
group("panic here", function() {
if (__ITER == 0) {
panic("here we panic");
}
console.log("here we don't");
})
}`,
)
require.NoError(t, err)

registry := metrics.NewRegistry()
builtinMetrics := metrics.RegisterBuiltinMetrics(registry)
r2, err := NewFromArchive(
&lib.TestPreInitState{
Logger: testutils.NewLogger(t),
BuiltinMetrics: builtinMetrics,
Registry: registry,
}, r1.MakeArchive())
require.NoError(t, err)

runners := map[string]*Runner{"Source": r1, "Archive": r2}
for name, r := range runners {
r := r
t.Run(name, func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

initVU, err := r.NewVU(ctx, 1, 1234, make(chan metrics.SampleContainer, 100))
require.NoError(t, err)

logger := logrus.New()
logger.SetLevel(logrus.InfoLevel)
logger.Out = io.Discard
hook := testutils.NewLogHook(
logrus.InfoLevel, logrus.ErrorLevel, logrus.FatalLevel, logrus.PanicLevel,
)
logger.AddHook(hook)

vu := initVU.Activate(&lib.VUActivationParams{RunContext: ctx})
activeVU, ok := vu.(*ActiveVU)
require.True(t, ok)
require.NoError(t, activeVU.Runtime.Set("panic", func(str string) { panic(str) }))
activeVU.state.Logger = logger

activeVU.Console.logger = logger.WithField("source", "console")
err = vu.RunOnce()
require.Error(t, err)
assert.Contains(t, err.Error(), "a panic occurred during JS execution: here we panic")
entries := hook.Drain()
require.Len(t, entries, 1)
assert.Equal(t, logrus.ErrorLevel, entries[0].Level)
require.True(t, strings.HasPrefix(entries[0].Message, "panic: here we panic"))
// broken since goja@f3cfc97811c0b4d8337902c3e42fb2371ba1d524 see
// https://github.com/dop251/goja/issues/179#issuecomment-783572020
// require.True(t, strings.HasSuffix(entries[0].Message, "Goja stack:\nfile:///script.js:3:4(12)"))

err = vu.RunOnce()
require.NoError(t, err)

entries = hook.Drain()
require.Len(t, entries, 1)
assert.Equal(t, logrus.InfoLevel, entries[0].Level)
require.Contains(t, entries[0].Message, "here we don't")
})
}
}

type multiFileTestCase struct {
fses map[string]fsext.Fs
rtOpts lib.RuntimeOptions
Expand Down
Loading