darwish commented 5 years ago

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 commented 5 years ago

Codecov Report

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

@@            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%)`	:arrow_up:

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 commented 5 years ago

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

DATA-DOG / go-sqlmock

adds missing lock around e.fulfilled() in ExpectationsWereMet() #151

Codecov Report