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

Data race in template writer when running concurrent tests with go test -race #703

Open
mscno opened this issue Nov 22, 2023 · 0 comments
Open

Comments

@mscno
Copy link

mscno commented Nov 22, 2023

When running several tests concurrently, the go race detection results in an error in the template writer code:

==================
WARNING: DATA RACE
Read at 0x0001067fb8e0 by goroutine 34:
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).getTemplate()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:135 +0x88
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).MustCompile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:95 +0x8c
  github.com/upper/db/v4/internal/sqladapter/exql.(*Statement).Compile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/statement.go:118 +0x204
  github.com/upper/db/v4/adapter/cockroachdb.(*database).CompileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:126 +0x4c
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).compileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:907 +0x250
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).StatementQuery()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:835 +0x2a4
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).IteratorContext()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:480 +0x34c
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).Iterator()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:470 +0x58
  github.com/upper/db/v4/adapter/cockroachdb.(*database).LookupName()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:161 +0x1c4
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).BindDB()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:544 +0x1c0
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:373 +0x8c
  github.com/upper/db/v4/internal/sqladapter.(*sqlAdapterWrapper).OpenDSN()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/sqladapter.go:56 +0x60
  github.com/upper/db/v4/adapter/cockroachdb.Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/cockroachdb.go:40 +0x3f4

...

Previous write at 0x0001067fb8e0 by goroutine 35:
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).getTemplate()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:136 +0xa4
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).MustCompile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:95 +0x8c
  github.com/upper/db/v4/internal/sqladapter/exql.(*Statement).Compile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/statement.go:118 +0x204
  github.com/upper/db/v4/adapter/cockroachdb.(*database).CompileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:126 +0x4c
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).compileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:907 +0x250
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).StatementQuery()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:835 +0x2a4
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).IteratorContext()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:480 +0x34c
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).Iterator()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:470 +0x58
  github.com/upper/db/v4/adapter/cockroachdb.(*database).LookupName()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:161 +0x1c4
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).BindDB()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:544 +0x1c0
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:373 +0x8c
  github.com/upper/db/v4/internal/sqladapter.(*sqlAdapterWrapper).OpenDSN()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/sqladapter.go:56 +0x60
  github.com/upper/db/v4/adapter/cockroachdb.Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/cockroachdb.go:40 +0x3f4

func (t *Template) getTemplate(k string) (*template.Template, bool) {
	t.templateMutex.RLock()
	defer t.templateMutex.RUnlock()

	if t.templateMap == nil { // Read done here in other go routine at the same time
		t.templateMap = make(map[string]*template.Template) // WRITE DONE TO t.templateMap here without read lock
	}

	v, ok := t.templateMap[k]
	return v, ok
}

func (t *Template) setTemplate(k string, v *template.Template) {
	t.templateMutex.Lock()
	defer t.templateMutex.Unlock()

	t.templateMap[k] = v
}

Can this be fixed by a simple PR wrapping the assignment of the empty map in a Write Lock (mux.Lock) ?

E.g. like so:

func (t *Template) getTemplate(k string) (*template.Template, bool) {
	t.templateMutex.RLock()
	defer t.templateMutex.RUnlock()

	if t.templateMap == nil { 
               t.templateMutex.Lock() // Lock prevents other routines from achieving a reader. Maybe add another check to see if it is still nil after getting the Write Lock?
   	       defer t.templateMutex.Unlock()
		t.templateMap = make(map[string]*template.Template)
	}

	v, ok := t.templateMap[k]
	return v, ok
}

func (t *Template) setTemplate(k string, v *template.Template) {
	t.templateMutex.Lock()
	defer t.templateMutex.Unlock()

	t.templateMap[k] = v
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant