From 59594877dafa901578dd80e390f2a25a236aaaeb Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 11:38:06 +0400 Subject: [PATCH 1/6] Fix unsafe concurrent SingularTable method call --- main.go | 4 +++- main_test.go | 33 +++++++++++++++++++++++++++++---- model_struct.go | 17 +++++++++++++++-- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/main.go b/main.go index fda63d29..cc8ac68c 100644 --- a/main.go +++ b/main.go @@ -12,6 +12,7 @@ import ( // DB contains information for current db connection type DB struct { + sync.Mutex Value interface{} Error error RowsAffected int64 @@ -170,7 +171,8 @@ func (s *DB) HasBlockGlobalUpdate() bool { // SingularTable use singular table by default func (s *DB) SingularTable(enable bool) { - modelStructsMap = sync.Map{} + s.parent.Lock() + defer s.parent.Unlock() s.parent.singularTable = enable } diff --git a/main_test.go b/main_test.go index ac40c32b..1dc30093 100644 --- a/main_test.go +++ b/main_test.go @@ -9,6 +9,7 @@ import ( "reflect" "strconv" "strings" + "sync" "testing" "time" @@ -277,6 +278,30 @@ func TestTableName(t *testing.T) { DB.SingularTable(false) } +func TestTableNameConcurrently(t *testing.T) { + DB := DB.Model("") + if DB.NewScope(Order{}).TableName() != "orders" { + t.Errorf("Order's table name should be orders") + } + + var wg sync.WaitGroup + wg.Add(10) + + for i := 1; i <= 10; i++ { + go func(db *gorm.DB) { + DB.SingularTable(true) + wg.Done() + }(DB) + } + wg.Wait() + + if DB.NewScope(Order{}).TableName() != "order" { + t.Errorf("Order's singular table name should be order") + } + + DB.SingularTable(false) +} + func TestNullValues(t *testing.T) { DB.DropTable(&NullValue{}) DB.AutoMigrate(&NullValue{}) @@ -1066,12 +1091,12 @@ func TestCountWithHaving(t *testing.T) { DB.Create(getPreparedUser("user1", "pluck_user")) DB.Create(getPreparedUser("user2", "pluck_user")) - user3:=getPreparedUser("user3", "pluck_user") - user3.Languages=[]Language{} + user3 := getPreparedUser("user3", "pluck_user") + user3.Languages = []Language{} DB.Create(user3) var count int - err:=db.Model(User{}).Select("users.id"). + err := db.Model(User{}).Select("users.id"). Joins("LEFT JOIN user_languages ON user_languages.user_id = users.id"). Joins("LEFT JOIN languages ON user_languages.language_id = languages.id"). Group("users.id").Having("COUNT(languages.id) > 1").Count(&count).Error @@ -1080,7 +1105,7 @@ func TestCountWithHaving(t *testing.T) { t.Error("Unexpected error on query count with having") } - if count!=2{ + if count != 2 { t.Error("Unexpected result on query count with having") } } diff --git a/model_struct.go b/model_struct.go index f646910a..8d6313fb 100644 --- a/model_struct.go +++ b/model_struct.go @@ -40,9 +40,11 @@ func (s *ModelStruct) TableName(db *DB) string { s.defaultTableName = tabler.TableName() } else { tableName := ToTableName(s.ModelType.Name()) + db.parent.Lock() if db == nil || (db.parent != nil && !db.parent.singularTable) { tableName = inflection.Plural(tableName) } + db.parent.Unlock() s.defaultTableName = tableName } } @@ -163,7 +165,18 @@ func (scope *Scope) GetModelStruct() *ModelStruct { } // Get Cached model struct - if value, ok := modelStructsMap.Load(reflectType); ok && value != nil { + isSingularTable := false + if scope.db != nil && scope.db.parent != nil { + scope.db.parent.Lock() + isSingularTable = scope.db.parent.singularTable + scope.db.parent.Unlock() + } + + hashKey := struct { + singularTable bool + reflectType reflect.Type + }{isSingularTable, reflectType} + if value, ok := modelStructsMap.Load(hashKey); ok && value != nil { return value.(*ModelStruct) } @@ -612,7 +625,7 @@ func (scope *Scope) GetModelStruct() *ModelStruct { } } - modelStructsMap.Store(reflectType, &modelStruct) + modelStructsMap.Store(hashKey, &modelStruct) return &modelStruct } From b4927348aebb1e84df37aa432c64ebb1c1ae3edb Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 11:40:05 +0400 Subject: [PATCH 2/6] gofmt --- preload_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/preload_test.go b/preload_test.go index 1a6a5d49..dd29fb5e 100644 --- a/preload_test.go +++ b/preload_test.go @@ -1677,7 +1677,7 @@ func TestPreloadManyToManyCallbacks(t *testing.T) { lvl := Level1{ Name: "l1", Level2s: []Level2{ - Level2{Name: "l2-1"}, Level2{Name: "l2-2"}, + {Name: "l2-1"}, {Name: "l2-2"}, }, } DB.Save(&lvl) From b923e78e811c9bf9a244c6fb0983443101a4332b Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 12:23:26 +0400 Subject: [PATCH 3/6] Verbose go get output --- wercker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wercker.yml b/wercker.yml index 0c3e73ef..43ad8209 100644 --- a/wercker.yml +++ b/wercker.yml @@ -83,7 +83,7 @@ build: code: | cd $WERCKER_SOURCE_DIR go version - go get -t ./... + go get -t -v ./... # Build the project - script: From 96d52f25b09fae789adb0c97ccf36f343a8f08fc Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 12:41:14 +0400 Subject: [PATCH 4/6] Use RWMutex --- main.go | 2 +- model_struct.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/main.go b/main.go index cc8ac68c..16820353 100644 --- a/main.go +++ b/main.go @@ -12,7 +12,7 @@ import ( // DB contains information for current db connection type DB struct { - sync.Mutex + sync.RWMutex Value interface{} Error error RowsAffected int64 diff --git a/model_struct.go b/model_struct.go index 8d6313fb..bfab49c0 100644 --- a/model_struct.go +++ b/model_struct.go @@ -40,11 +40,11 @@ func (s *ModelStruct) TableName(db *DB) string { s.defaultTableName = tabler.TableName() } else { tableName := ToTableName(s.ModelType.Name()) - db.parent.Lock() + db.parent.RLock() if db == nil || (db.parent != nil && !db.parent.singularTable) { tableName = inflection.Plural(tableName) } - db.parent.Unlock() + db.parent.RUnlock() s.defaultTableName = tableName } } @@ -167,9 +167,9 @@ func (scope *Scope) GetModelStruct() *ModelStruct { // Get Cached model struct isSingularTable := false if scope.db != nil && scope.db.parent != nil { - scope.db.parent.Lock() + scope.db.parent.RLock() isSingularTable = scope.db.parent.singularTable - scope.db.parent.Unlock() + scope.db.parent.RUnlock() } hashKey := struct { From cd0f3ae41a86cdd5884e14147336542a81294fd6 Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 12:41:23 +0400 Subject: [PATCH 5/6] Run tests with race detector --- wercker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wercker.yml b/wercker.yml index 43ad8209..de351fd2 100644 --- a/wercker.yml +++ b/wercker.yml @@ -95,7 +95,7 @@ build: - script: name: test sqlite code: | - go test ./... + go test -race -v ./... - script: name: test mariadb From ef9d2070bbed3d9186f8e0aa1b86c55b20411a55 Mon Sep 17 00:00:00 2001 From: Emir Beganovic Date: Sun, 14 Apr 2019 12:46:05 +0400 Subject: [PATCH 6/6] Run tests with race detector --- wercker.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/wercker.yml b/wercker.yml index de351fd2..98234583 100644 --- a/wercker.yml +++ b/wercker.yml @@ -100,49 +100,49 @@ build: - script: name: test mariadb code: | - GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mariadb:3306)/gorm?charset=utf8&parseTime=True" go test ./... + GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mariadb:3306)/gorm?charset=utf8&parseTime=True" go test -race ./... - script: name: test mysql5.7 code: | - GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql57:3306)/gorm?charset=utf8&parseTime=True" go test ./... + GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql57:3306)/gorm?charset=utf8&parseTime=True" go test -race ./... - script: name: test mysql5.6 code: | - GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql56:3306)/gorm?charset=utf8&parseTime=True" go test ./... + GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql56:3306)/gorm?charset=utf8&parseTime=True" go test -race ./... - script: name: test mysql5.5 code: | - GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql55:3306)/gorm?charset=utf8&parseTime=True" go test ./... + GORM_DIALECT=mysql GORM_DSN="gorm:gorm@tcp(mysql55:3306)/gorm?charset=utf8&parseTime=True" go test -race ./... - script: name: test postgres code: | - GORM_DIALECT=postgres GORM_DSN="host=postgres user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test ./... + GORM_DIALECT=postgres GORM_DSN="host=postgres user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test -race ./... - script: name: test postgres96 code: | - GORM_DIALECT=postgres GORM_DSN="host=postgres96 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test ./... + GORM_DIALECT=postgres GORM_DSN="host=postgres96 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test -race ./... - script: name: test postgres95 code: | - GORM_DIALECT=postgres GORM_DSN="host=postgres95 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test ./... + GORM_DIALECT=postgres GORM_DSN="host=postgres95 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test -race ./... - script: name: test postgres94 code: | - GORM_DIALECT=postgres GORM_DSN="host=postgres94 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test ./... + GORM_DIALECT=postgres GORM_DSN="host=postgres94 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test -race ./... - script: name: test postgres93 code: | - GORM_DIALECT=postgres GORM_DSN="host=postgres93 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test ./... + GORM_DIALECT=postgres GORM_DSN="host=postgres93 user=gorm password=gorm DB.name=gorm port=5432 sslmode=disable" go test -race ./... - script: name: test mssql code: | - GORM_DIALECT=mssql GORM_DSN="sqlserver://gorm:LoremIpsum86@mssql:1433?database=gorm" go test ./... + GORM_DIALECT=mssql GORM_DSN="sqlserver://gorm:LoremIpsum86@mssql:1433?database=gorm" go test -race ./...