Skip to content

Conversation

@darwish
Copy link

@darwish darwish commented Dec 20, 2018

If the query runs in a separate goroutine from the one that ExpectationsWereMet is called in, the race detector finds an unsynchronized access to the e.triggered variable.

This can be tested by running the test added in this PR without the fix that the PR adds:

go test -race -run=TestQueryWithTimeout github.com/DATA-DOG/go-sqlmock

and you end up with something like:

WARNING: DATA RACE
Read at 0x00c4200d8108 by goroutine 6:
  github.com/DATA-DOG/go-sqlmock.(*ExpectedQuery).fulfilled()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/expectations.go:28 +0x4c
  github.com/DATA-DOG/go-sqlmock.(*sqlmock).ExpectationsWereMet()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/sqlmock.go:170 +0xc5
  github.com/DATA-DOG/go-sqlmock.TestQueryWithTimeout()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/sqlmock_test.go:1193 +0x4df
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Previous write at 0x00c4200d8108 by goroutine 9:
  github.com/DATA-DOG/go-sqlmock.(*sqlmock).query()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/sqlmock.go:481 +0x5fe
  github.com/DATA-DOG/go-sqlmock.(*sqlmock).QueryContext()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/sqlmock_go18.go:23 +0x234
  database/sql.ctxDriverQuery()
      /usr/local/go/src/database/sql/ctxutil.go:48 +0x2e2
  database/sql.(*DB).queryDC.func1()
      /usr/local/go/src/database/sql/sql.go:1464 +0x2d9
  database/sql.withLock()
      /usr/local/go/src/database/sql/sql.go:3032 +0x74
  database/sql.(*DB).queryDC()
      /usr/local/go/src/database/sql/sql.go:1459 +0x951
  database/sql.(*DB).query()
      /usr/local/go/src/database/sql/sql.go:1442 +0x183
  database/sql.(*DB).QueryContext()
      /usr/local/go/src/database/sql/sql.go:1419 +0xe2
  database/sql.(*DB).Query()
      /usr/local/go/src/database/sql/sql.go:1433 +0xa8
  github.com/DATA-DOG/go-sqlmock.queryWithTimeout.func1()
      /gopath/src/github.com/DATA-DOG/go-sqlmock/sqlmock_test.go:1203 +0x7a

@codecov-io
Copy link

codecov-io commented Dec 20, 2018

Codecov Report

Merging #151 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   91.21%   91.25%   +0.03%     
==========================================
  Files          13       13              
  Lines         717      720       +3     
==========================================
+ Hits          654      657       +3     
  Misses         44       44              
  Partials       19       19
Impacted Files Coverage Δ
sqlmock.go 93.83% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7b0b93...e671f17. Read the comment docs.

@l3pp4rd
Copy link
Member

l3pp4rd commented Dec 21, 2018

thanks! probably I need to think how to simplify the locking in general, easy to make mistakes like this.

@l3pp4rd l3pp4rd merged commit 2a5889f into DATA-DOG:master Dec 21, 2018
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

Successfully merging this pull request may close these issues.

4 participants