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

catching of VU panics #1697

Merged
merged 2 commits into from
Nov 2, 2020
Merged

catching of VU panics #1697

merged 2 commits into from
Nov 2, 2020

Conversation

mstoykov
Copy link
Contributor

No description provided.

@mstoykov mstoykov requested review from imiric and na-- October 29, 2020 17:21
@codecov-io
Copy link

codecov-io commented Oct 29, 2020

Codecov Report

Merging #1697 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1697      +/-   ##
==========================================
+ Coverage   72.31%   72.33%   +0.01%     
==========================================
  Files         176      176              
  Lines       13536    13538       +2     
==========================================
+ Hits         9789     9793       +4     
+ Misses       3134     3132       -2     
  Partials      613      613              
Flag Coverage Δ
ubuntu 72.30% <100.00%> (-0.01%) ⬇️
windows 70.88% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/runner.go 80.45% <100.00%> (+0.32%) ⬆️
js/modules/k6/http/request.go 80.64% <0.00%> (-0.07%) ⬇️
cmd/cloud.go 6.85% <0.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7586ddf...d1db643. Read the comment docs.

@mstoykov mstoykov changed the title WIP catching of VU panics catching of VU panics Oct 30, 2020
@mstoykov mstoykov added this to the v0.29.0 milestone Oct 30, 2020
@mstoykov
Copy link
Contributor Author

This supersedes #1665

@mstoykov
Copy link
Contributor Author

For the record this code resets the stack and other stuff to the previous values if there is a panic (even if the code below panics again) to what it was before a function was called and calling a Callable basically calls this.

So unless this code panics in the middle the state of the Runtime should be "somewhat" restored to the previous one. Obviously any changes to global variables will stick.

And if the panic in somehow has gotten the non goja stack but for example some internal to k6 state in a bad state that will lead to problems. I can't figure out a way to break the goja stack in my first few tries and ... I can't remember anything in k6 that will have such a problem so it seems safe.

js/runner.go Outdated Show resolved Hide resolved
js/runner_test.go Outdated Show resolved Hide resolved
js/runner.go Show resolved Hide resolved
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash when merging and close #1601 and #1665

@mstoykov mstoykov merged commit c51095a into master Nov 2, 2020
@mstoykov mstoykov deleted the handleVUPanics branch November 2, 2020 13:23
salem84 pushed a commit to salem84/k6 that referenced this pull request Nov 5, 2020
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 this pull request may close these issues.

4 participants