Panic in the new exec cache code #214
Labels
Comments
|
Hm... I can't see any way of This looks like a data race to me (which could be the reason for the original panic you saw in #212 as well). |
|
Were you able to get to the bottom of this? |
|
Sorry, I haven't had any real leads until yesterday when ... I think I hit it, but I am not certain it is the same thing as I changed stuff ... and the current thing that looks wrong is that goja.Program somehow shares stuff... which seems unlikely to me. My change was that instead of having a single goja.Runtime running babel for us, we will have a single goja.Program parsed by goja.Compile and then a separate goja.Runtime which runs the goja.Program for each instance that we need it (with a lot of asterisks all over the place obviously). Given that we do something similar in other places .... this seems completely unlikely to me and I guess it is something completely different, that this change just made more likely or something. Also, this only happens if the test (go test) actually ends prematurely ... so 🤷 Unfortunately, I have to prioritize another thing before I come back to look at it again, as after 1-2 hours of running test after test I was nowhere .. I am going to try again in January, and hopefully come back to you :D |
|
I think I ... found it, although I have no idea why, except that it seems to be a much bigger problem. The following code is racing (fairly consistently, but you might need to run it more than once). Edit: simpler example in the next comment func TestPanic(t *testing.T) {
// r, _ := http.Get("https://unpkg.com/@babel/standalone/babel.min.js")
r, _ := http.Get("https://raw.githubusercontent.com/loadimpact/k6/master/js/compiler/lib/babel.min.js")
b, _ := ioutil.ReadAll(r.Body)
s := MustCompile("babel.min.js", string(b), false)
fmt.Println(len(b))
for i := 0; i < 10; i++ {
go func() {
vm := New()
for i := 0; i < 10; i++ {
_, err := vm.RunProgram(s)
if err != nil {
panic(err)
}
}
}()
}
}Bisecting goja ... obviously leads to b2a8925 but IMO this is just what made it noticeable, and the bug is actually that You might need to follow golang/go#10661 (comment) to get the full stacks, btw. The actual race on my machine( but I have seen smaller and they change so it is not the same code always): |
|
I guess the reason is that regexp literals will be compiled to |
|
I did some more digging and it seems to be Line 43 in 6b6d5e2 Using edit: but it looks like @lujjjh is correct. The following Line 1454 in 6b6d5e2 Line 59 in 6b6d5e2 I still find it strange that we have |
|
Literals... of course! Thanks for finding out, I'll fix it shortly. As for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I haven't been able to reproduce this and I did try some sketchy uses including
Also unfortunately the stacktrace isn't full, although what is in it is probably the helpful part given that it still wouldn't have provided us with the source code :( :
This is with a8e472c and as far as I can see
cache.targetisnilbut given howr.cacheis set this is impossible ?Maybe it is a corrupt address which seems ... just as unlikely 🤔
The text was updated successfully, but these errors were encountered: