Skip to content

Commit

Permalink
Don't compile and transform all scripts for each VU if archive
Browse files Browse the repository at this point in the history
This was a regression introduced with 3506ee1. The core issue lies with
a difference in how we run archive files and not archived scripts.

If the script is not archived we run it once in order to gather all the
files that will be required and to calculate the options among other
things. This is not required for archives as it was done when the
archive was created, so this step was skipped and the fist time the
files were executed was when a VU was created. This should've lead to
executing and loading each file for each VU previously as well if not
for the fact that all the code is in map. That map is first generated in
the original `bundle` of source codes and than copied to a new bundle
when a VU is created. Or more accurately it is now copied, previously
just a reference to the map was copied.
All of this together lead to the fact that the first VU that gets
started will populate the map and all VUs after that would've used it.
After it is copied this didn't happen.
Obviosly this will have rece condition of VUs where initialized
concurently, but they aren't. But PR#1007 also initializes VUs
concurrently which would've lead to ... probably some panics ;(.

The current fix is the minimalist somewhat concurrent and future proof
fix that is to just run the script once when we load it from the archive
in order to just cache all the compilation and transformation.

Maybe in the future we can use that now calculated values although I
have the sneaking suspicion that there will be dragons once we start
testing with things like enviromental variables and complex configs, not
to speak of precedences of configurations.
  • Loading branch information
mstoykov committed Jun 12, 2019
1 parent 607b63c commit b8ea29d
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 16 deletions.
8 changes: 6 additions & 2 deletions js/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,18 @@ func NewBundleFromArchive(arc *lib.Archive, rtOpts lib.RuntimeOptions) (*Bundle,
env[k] = v
}

return &Bundle{
bundle := &Bundle{
Filename: arc.Filename,
Source: string(arc.Data),
Program: pgm,
Options: arc.Options,
BaseInitContext: initctx,
Env: env,
}, nil
}
if err := bundle.instantiate(bundle.BaseInitContext.runtime, bundle.BaseInitContext); err != nil {
return nil, err
}
return bundle, nil
}

func (b *Bundle) makeArchive() *lib.Archive {
Expand Down
18 changes: 4 additions & 14 deletions js/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1471,20 +1471,10 @@ func TestArchiveNotPanicking(t *testing.T) {
arc := r1.MakeArchive()
arc.Files = make(map[string][]byte)
r2, err := NewFromArchive(arc, lib.RuntimeOptions{})
require.NoError(t, err)

runners := map[string]*Runner{"Source": r1, "Archive": r2}
for name, r := range runners {
t.Run(name, func(t *testing.T) {
ch := make(chan stats.SampleContainer, 100)
_, err := r.NewVU(ch)
if name == "Source" {
require.NoError(t, err)
} else {
require.Error(t, err)
}
})
}
// we do want this to error here as this is where we find out that a given file is not in the
// archive
require.Error(t, err)
require.Nil(t, r2)
}

func TestStuffNotPanicking(t *testing.T) {
Expand Down

0 comments on commit b8ea29d

Please sign in to comment.