Fix data race: collect results, then append.

[Deleplace]

Fixes #871

I preserved the apparent semantics, i.e. the order in which results are appended to InjectedFiles.

[kzc]

Good stuff. Until this issue I didn't know that Go has a built in race detector. Might as well use it in testing by default.

--- a/Makefile
+++ b/Makefile
@@ -22,3 +22,3 @@ check-go-version:
 test-go:
-       go test ./internal/...
+       go test -race ./internal/...

[Deleplace]

Yes, and the race detector is awesome!

To be clear, it doesn't do any static analysis of the code. It compiles a special instrumented version of the package, which keeps track at runtime of all of the read and write memory accesses of the current execution. Then when the program is run (or when the tests are run) and a violation of the memory model happens, it detects it and reports it.

[kzc]

With this PR and the patch above I'm still seeing a test failure due to a race condition:

$ make clean test
...
fatal error: too many address space collisions for -race mode
...
FAIL	github.com/evanw/esbuild/internal/css_parser	0.017s

I'm using:

go version go1.15.7 darwin/amd64

[kzc]

Just to be clear, this error no longer occurs on my machine with this PR:

FAIL    github.com/evanw/esbuild/internal/bundler       3.027s

[Deleplace]

The race detector does incur a large overhead in memory and compute. It may make sense to use -race in the Makefile, and that should be discussed in its own issue.

The data we have about the current PR is positive: sensible diff, fixes the problem on my machine, passes the CI tests. The current PR doesn't add any automated use of the race detector.

[kzc]

@Deleplace So you don't see the internal/css_parser failure in #872 (comment)? Would you mind sharing the output of go version on your machine?

Edit: Never mind - resolved below.

[kzc]

The fatal error I'm seeing in go test -race ./internal/css_parser is a known issue on the older now unsupported macos 10.10, although it was a supported OS version at the time of the following bug report in 2018:

golang/go#26475 (comment)

The stack trace in the link is identical to the one on my machine.

Edit 1: If I downgrade to go version go1.13.5 darwin/amd64 with this commit present then go test -race ./internal/css_parser and the rest of the tests in go test -race ./internal/... pass cleanly. Of course all the wasm tests fail, but that is to be expected with this old version of go. Because darwin 10.10 was no longer supported by go 1.14, the Go team reverted the aforementioned commit, which broke -race support on darwin 10.10.

Edit 2: Long story short - my local go test -race ./internal/css_parser failure with go version go1.15.7 darwin/amd64 was unrelated to this PR.

[evanw]

Thanks so much for finding this, and for the fix. Looks good to me.

[evanw]

Wait actually now that I look at this more, I'm confused about why this is an issue. Sorry for not taking a closer look first. The array should already be big enough since there is this at the top:

injectedFiles := make([]config.InjectedFile, 0, len(s.options.InjectedDefines)+len(s.options.InjectAbsPaths))

The fix in this PR isn't quite right because the continue statements in the loop now leave holes in the array. In addition, the problem isn't about reallocation at all.

Edit: Maybe the race is with the capacity field in the slice, and not with the data in the slice at all? Perhaps the bounds check in injectedFiles[i] is the problem. It looks like fixing that instead like this fixes the problem:

diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go
index dac5dfee..30f902e1 100644
--- a/internal/bundler/bundler.go
+++ b/internal/bundler/bundler.go
@@ -1081,10 +1081,10 @@ func (s *scanner) preprocessInjectedFiles() {
 
                // Wait for the results in parallel
                injectWaitGroup.Add(1)
-               go func(i int, prettyPath string, resolveResult *resolver.ResolveResult) {
-                       injectedFiles[i] = <-channel
+               go func(file *config.InjectedFile) {
+                       *file = <-channel
                        injectWaitGroup.Done()
-               }(i, prettyPath, resolveResult)
+               }(&injectedFiles[i])
        }
 
        injectWaitGroup.Wait()

[Deleplace]

For the "why was this an issue" in the first place:

• see comment #871: we were doing several hazardous things concurrently
• the race detector complained, and it is designed to not report any false positive
• I understand the rationale "in this case there should not be any reallocation", however it is still a memory model violation to be doing files = append(files, f) and files[i] = <-channel concurrently. This is not just theoretical, the compiler does many weird things with the assumption that the code is not racy, and bad things may happen anytime, today, or next month.

[Deleplace]

For the "leave holes in the array", I don't think we have them. Consider:

• a new distinct slice results which has fixed length M = len(s.options.InjectAbsPaths)
• j is incremented only when 1 extra computation has just been launched
• after all the computations have finished, only the sublist results[:j] is used
• we alter injectedFiles only once, after all the computations have finished to write to results[:j]

[evanw]

Ah right there are no holes because of the results[:j]. 👍