Silent wrong interrupted error for interrupted iteration

[cuonglm]

goja Interrupt only interrupts the JS vm, not the Go code which runs it.
So there might be a race condition that an interrupted iteration was not
probably detected, especially in case of heavy load.

Detect that case and silent the interrupted error.

Fixes #1283

[codecov-io]

Codecov Report

Merging #1284 into new-schedulers will decrease coverage by 0.02%.
The diff coverage is 0%.

@@                Coverage Diff                 @@
##           new-schedulers    #1284      +/-   ##
==================================================
- Coverage           74.92%   74.89%   -0.03%     
==================================================
  Files                 159      159              
  Lines               12288    12290       +2     
==================================================
- Hits                 9207     9205       -2     
- Misses               2590     2593       +3     
- Partials              491      492       +1

Impacted Files	Coverage Δ
js/runner.go	82.5% <0%> (-0.64%)	⬇️
lib/executor/vu_handle.go	90% <0%> (-5.72%)	⬇️
lib/helpers.go	95.83% <0%> (-4.17%)	⬇️
lib/executor/variable_looping_vus.go	91.62% <0%> (+0.93%)	⬆️
lib/executor/per_vu_iterations.go	96.55% <0%> (+2.29%)	⬆️

---

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 f6be35c...bb93559. Read the comment docs.

[na--]

I'm confused... 😕 We're interrupting the JS VM only after the VU context is done: https://github.com/loadimpact/k6/blob/f6be35cfa006c56954d6821e0f94f4863f063e45/js/runner.go#L316-L319

But isFullIteration is true only if the VU context isn't done: https://github.com/loadimpact/k6/blob/f6be35cfa006c56954d6821e0f94f4863f063e45/js/runner.go#L483-L489

So how come your fix, which checks both of these things, work? https://github.com/loadimpact/k6/blob/bb935595d622d99006bad431cf0eee3b3ec84b23/js/runner.go#L444-L446

Something smells fishy and I think we have a bigger underlying issue here 😕

[cuonglm]

@na-- the one you linked above are in runPart, what’s actually happened in RunOnce. But the main thing here is we interrupt the VU.Runtime and checking ctx.Done (both in RunOnce and runFn) concurrently, that leads to the racy behavior.

If you see the other place where we call runFn, there’s also a check like I added.

[na--]

hmm I'm still not sure I completely grok what's happening here... 😕 Something seems fishy, because your fix, as well as the one you pointed out in runPart() just seem like they're masking the race instead of fixing it: https://github.com/loadimpact/k6/blob/f6be35cfa006c56954d6821e0f94f4863f063e45/js/runner.go#L328-L337

If we call interrupt only after the context is done: https://github.com/loadimpact/k6/blob/f6be35cfa006c56954d6821e0f94f4863f063e45/js/runner.go#L407-L411

And if we set isFullIteration to true only if the context isn't done, but the err result is before that check: https://github.com/loadimpact/k6/blob/f6be35cfa006c56954d6821e0f94f4863f063e45/js/runner.go#L480-L489

How is it possible that we have both a full iteration and an interrupted one at the same time? https://github.com/loadimpact/k6/blob/bb935595d622d99006bad431cf0eee3b3ec84b23/js/runner.go#L444-L446

Just to have those two things in the same if statement seems like a contradiction... 😕

[cuonglm]

@na-- That's a race, so you can't guarantee anything. The main point is that you run the interrupt inside a goroutine, so it's completely upto scheduler to schedule them to run. I add some logrus.Debug lines and see that whenever this happen, isFullIteration is true and the call JS runtime return an error.

If you see commit message in a2e1c6f, @mstoykov also mention about this problem.

[na--]

Well, if it's a race, shouldn't we fix it, instead of working around it? The isFullIteration && gErr.Value() == errInterrupt check is obviously fishy - an iteration cannot be both Full and also interrupted at the same time. If both of these conditions are true, then one of them doesn't make sense and has to be fixed...

If you see commit message in a2e1c6f, @mstoykov also mention about this problem.

I'm not sure that's the same problem. His change fixed a panic that occurs when the number of VUs is rapidly changed via the k6 REST API. Now that I think about it, I'm not sure his change is actually necessary in the new-schedulers branch...