Skip to content

Commit

Permalink
compilepkg: fix race when run without sandbox
Browse files Browse the repository at this point in the history
On platforms without sandbox execution support such as Windows, multiple
go_test targets declared in the same package would cause a race condition.

```
[
    go_library(
        name = "same_package_{}".format(i),
        srcs = ["same_package.go"],
        importpath = "github.com/bazelbuild/rules_go/tests/core/go_test/same_package_{}".format(i),
        x_defs = {
            "name": "{}".format(i),
        },
    )
    for i in range(1, 80)
]

[
    go_test(
        name = "same_package_{}_test".format(i),
        srcs = ["same_package_test.go"],
        embed = [":same_package_{}".format(i)],
    )
    for i in range(1, 80)
]
```

For each of go_test targets, there would be internal archive and
external archive that needs to compile from source files.  However, due
to testfilter, external archive would end up without any Go source and
thus compilepkg needs to generate a dummy source file `_empty.go` to
trick 'go tool compile'.

This `_empty.go` file is generated in the output path of the Bazel
package thus multiple go_test targets in the same package would generate
this file using the same path.  On a sandbox supported platform,
this is not a problem as the full path is prefixed with the sandbox root
dir.  However, without sandbox, the full path is the same for each
go_test's external archive compilation, leading to a race condition
where multiple compilations running in parallel would compete to
create/delete the same file.

Fix this by using the unique importPath to create a directory to store
the _empty.go file.  This would effectively give each go_test compilation
it's own _empty.go file in non-sandbox environment.

Worth to note that in current solution, we still have yet to solve
problem when 'importPath' is not provided. For example, in a setup like
this where `go_library` does not exist, `importPath` will be blank and
thus causes similar issues on non-sandbox platforms.

```
[
    go_test(
        name = "same_package_{}_test".format(i),
        srcs = ["same_package_test.go"],
    )
    for i in range(1, 80)
]
```

Although this is a valid use case, we recommend avoid setting up tests
this way. Instead, you could setup a single go_test target with multiple
shards to achieve similar result:

```
go_test(
    name = "same_package_{}_test".format(i),
    srcs = ["same_package_test.go"],
    shard_count = 80,
)
```

For more information, please review Bazel Test Sharding documentation(1)

(1): https://docs.bazel.build/versions/main/test-encyclopedia.html#test-sharding
  • Loading branch information
sluongng committed May 11, 2022
1 parent fee2095 commit 8e809f5
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 3 deletions.
16 changes: 13 additions & 3 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,19 +188,29 @@ func compileArchive(
// We need to run the compiler to create a valid archive, even if there's
// nothing in it. GoPack will complain if we try to add assembly or cgo
// objects.
//
// _empty.go needs to be in a deterministic location (not tmpdir) in order
// to ensure deterministic output
emptyPath := filepath.Join(filepath.Dir(outPath), "_empty.go")
// to ensure deterministic output. The location also needs to be unique
// otherwise platforms without sandbox support may race to create/remove
// the file during parallel compilation.
emptyDir := filepath.Join(filepath.Dir(outPath), sanitizePathForIdentifier(importPath))
err := os.Mkdir(emptyDir, 0700)
if err != nil {
return err
}
defer os.RemoveAll(emptyDir)
emptyPath := filepath.Join(emptyDir, "_empty.go")
if err := ioutil.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
return err
}
defer os.Remove(emptyPath)

srcs.goSrcs = append(srcs.goSrcs, fileInfo{
filename: emptyPath,
ext: goExt,
matched: true,
pkg: "empty",
})
defer os.Remove(emptyPath)
}
packageName := srcs.goSrcs[0].pkg
var goSrcs, cgoSrcs []string
Expand Down
21 changes: 21 additions & 0 deletions tests/core/go_test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,27 @@ go_library(
deps = [":indirect_import_lib"],
)

[
go_library(
name = "same_package_{}".format(i),
srcs = ["same_package.go"],
importpath = "github.com/bazelbuild/rules_go/tests/core/go_test/same_package_{}".format(i),
x_defs = {
"name": "{}".format(i),
},
)
for i in range(1, 80)
]

[
go_test(
name = "same_package_{}_test".format(i),
srcs = ["same_package_test.go"],
embed = [":same_package_{}".format(i)],
)
for i in range(1, 80)
]

go_bazel_test(
name = "testmain_without_exit_test",
srcs = ["testmain_without_exit_test.go"],
Expand Down
3 changes: 3 additions & 0 deletions tests/core/go_test/same_package.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package same_package

var name string
11 changes: 11 additions & 0 deletions tests/core/go_test/same_package_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package same_package

import (
"testing"
)

func OkTest(t *testing.T) {
if name == "" {
t.Errorf("expected name to be non empty")
}
}

0 comments on commit 8e809f5

Please sign in to comment.