Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make disableEager not trigger the race detector #746

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions connection.go
Expand Up @@ -241,8 +241,13 @@ func (c *Connection) Q() *Query {

// disableEager disables eager mode for current connection.
func (c *Connection) disableEager() {
c.eager = false
c.eagerFields = []string{}
// The check technically is not required, because (*Connection).Eager() creates a (shallow) copy.
// When not reusing eager connections, this should be safe.
// However, this write triggers the go race detector.
if c.eager {
c.eager = false
c.eagerFields = []string{}
}
}

// TruncateAll truncates all data from the datasource
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Expand Up @@ -13,7 +13,7 @@ services:
ports:
- "3306:3306"
postgres:
image: postgres:9.6
image: postgres:10.21
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous test failure was related to some postgres bug. As 9.6 is not supported anymore, I bumped to a supported version.

environment:
- POSTGRES_DB=pop_test
- POSTGRES_USER=postgres
Expand Down
14 changes: 14 additions & 0 deletions executors_test.go
@@ -1,6 +1,7 @@
package pop

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -533,6 +534,19 @@ func Test_Create_Non_PK_ID(t *testing.T) {
})
}

func Test_Create_Parallel(t *testing.T) {
Copy link
Contributor Author

@zepatrik zepatrik Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see, this test failed first with the data race: https://github.com/gobuffalo/pop/runs/7304729964?check_suite_focus=true#step:6:47

==================
WARNING: DATA RACE
Read at 0x00c000074ca0 by goroutine 103:
  github.com/gobuffalo/pop/v6.(*Connection).Create()
      /home/runner/work/pop/pop/executors.go:176 +0x90
  github.com/gobuffalo/pop/v6.Test_Create_Parallel.func1.1()
      /home/runner/work/pop/pop/executors_test.go:546 +0x147
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.3/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.3/x64/src/testing/testing.go:1486 +0x47
Previous write at 0x00c000074ca0 by goroutine 80:
  github.com/gobuffalo/pop/v6.(*Connection).disableEager()
      /home/runner/work/pop/pop/connection.go:244 +0xbc
  github.com/gobuffalo/pop/v6.(*Connection).Create()
      /home/runner/work/pop/pop/executors.go:178 +0xa1
  github.com/gobuffalo/pop/v6.Test_Create_Parallel.func1.1()
      /home/runner/work/pop/pop/executors_test.go:546 +0x147
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.3/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.3/x64/src/testing/testing.go:1486 +0x47

if PDB == nil {
t.Skip("skipping integration tests")
}
for i := 0; i < 5; i++ {
i := i
t.Run(fmt.Sprintf("case=%d", i), func(t *testing.T) {
t.Parallel()
require.NoError(t, PDB.Create(&CrookedColour{Name: fmt.Sprintf("Singer %d", i)}))
})
}
}

func Test_Embedded_Struct(t *testing.T) {
if PDB == nil {
t.Skip("skipping integration tests")
Expand Down