Skip to content
This repository has been archived by the owner on Jun 19, 2019. It is now read-only.

Fix potential data race in Column.fieldPath #21

Merged
merged 1 commit into from
Feb 6, 2017

Conversation

yonekawa
Copy link
Contributor

@yonekawa yonekawa commented Feb 6, 2017

xorm's core has potential data race here

For example, my test code(simplified) with parallel.

var db *xorm.Engine
func TestMain(m *testing.M) {
    db, _ = xorm.NewEngine("mysql", "...")
}

func TestA(t *testing.T) {
	t.Parallel()

	_, err := db.Where("name = ?", "A").Delete(Struct{})
	if err != nil {
		t.Error(err)
		t.FailNow()
	}
}

func TestB(t *testing.T) {
	t.Parallel()

	_, err := db.Where("name = ?", "B").Delete(Struct{})
	if err != nil {
		t.Error(err)
		t.FailNow()
	}
}

When I run go test with -race option, detect DATA RACE.

WARNING: DATA RACE
Read at 0x00c4201583e0 by goroutine 7:
  github.com/yonekawa/example/vendor/github.com/go-xorm/core.(*Column).ValueOfV()
      /Users/yonekawa/project/vendor/github.com/go-xorm/core/column.go:138 +0x250
  github.com/yonekawa/example/vendor/github.com/go-xorm/core.(*Column).ValueOf()
      /Users/yonekawa/project/vendor/github.com/go-xorm/core/column.go:119 +0xa1
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.buildConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:518 +0x374
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Statement).buildConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:1104 +0x207
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Statement).genConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:1110 +0x20c
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Session).Delete()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/session_delete.go:100 +0x314
  github.com/yonekawa/example/core/infra/database/a.TestB()
      /Users/yonekawa/project/core/infra/database/a/connection_test.go:23 +0x1f4
  testing.tRunner()
      /usr/local/Cellar/go/1.7.4_1/libexec/src/testing/testing.go:610 +0xc9

Previous write at 0x00c4201583e0 by goroutine 6:
  strings.genSplit()
      /usr/local/Cellar/go/1.7.4_1/libexec/src/strings/strings.go:259 +0x390
  strings.Split()
      /usr/local/Cellar/go/1.7.4_1/libexec/src/strings/strings.go:287 +0x72
  github.com/yonekawa/example/vendor/github.com/go-xorm/core.(*Column).ValueOfV()
      /Users/yonekawa/project/vendor/github.com/go-xorm/core/column.go:125 +0xe67
  github.com/yonekawa/example/vendor/github.com/go-xorm/core.(*Column).ValueOf()
      /Users/yonekawa/project/vendor/github.com/go-xorm/core/column.go:119 +0xa1
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.buildConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:518 +0x374
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Statement).buildConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:1104 +0x207
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Statement).genConds()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/statement.go:1110 +0x20c
  github.com/yonekawa/example/vendor/github.com/go-xorm/xorm.(*Session).Delete()
      /Users/yonekawa/project/vendor/github.com/go-xorm/xorm/session_delete.go:100 +0x314
  github.com/yonekawa/example/core/infra/database/a.TestA()
      /Users/yonekawa/project/core/infra/database/a/connection_test.go:13 +0x1f4
  testing.tRunner()
      /usr/local/Cellar/go/1.7.4_1/libexec/src/testing/testing.go:610 +0xc9

I think we don't need to cache fieldPath into Column.
Because, same logic is exists here without cache result.

@lunny lunny added the bug label Feb 6, 2017
@lunny lunny merged commit 7daacb2 into go-xorm:master Feb 6, 2017
@lunny
Copy link
Member

lunny commented Feb 6, 2017

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants