{"action":"created","issue":{"url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/61","repository_url":"https://api.github.com/repos/DATA-DOG/go-sqlmock","labels_url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/61/labels{/name}","comments_url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/61/comments","events_url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/61/events","html_url":"https://github.com/DATA-DOG/go-sqlmock/issues/61","id":193395239,"number":61,"title":"Race on ExpectQuery and Query","user":{"login":"bradleyfalzon","id":1834577,"avatar_url":"https://avatars.githubusercontent.com/u/1834577?v=3","gravatar_id":"","url":"https://api.github.com/users/bradleyfalzon","html_url":"https://github.com/bradleyfalzon","followers_url":"https://api.github.com/users/bradleyfalzon/followers","following_url":"https://api.github.com/users/bradleyfalzon/following{/other_user}","gists_url":"https://api.github.com/users/bradleyfalzon/gists{/gist_id}","starred_url":"https://api.github.com/users/bradleyfalzon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bradleyfalzon/subscriptions","organizations_url":"https://api.github.com/users/bradleyfalzon/orgs","repos_url":"https://api.github.com/users/bradleyfalzon/repos","events_url":"https://api.github.com/users/bradleyfalzon/events{/privacy}","received_events_url":"https://api.github.com/users/bradleyfalzon/received_events","type":"User","site_admin":false},"labels":[{"id":79196208,"url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/labels/bug","name":"bug","color":"fc2929","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2016-12-05T02:00:30Z","updated_at":"2016-12-05T11:44:05Z","closed_at":null,"body":"Hi,\r\n\r\nWe have an application that we're using with go-sqlmock, we're receiving a race warning using release d4cd2ca2ad1cc2130bba385aab072218f131f636.\r\n\r\nOutput from go test -race\r\n\r\n```==================\r\nWARNING: DATA RACE\r\nWrite at 0x00c420066e68 by goroutine 32:\r\n  domain.example.com/group/project/vendor/gopkg.in/DATA-DOG/go-sqlmock%2ev1.(*sqlmock).ExpectQuery()\r\n      /u01/dev/go/src/domain.example.com/group/project/vendor/gopkg.in/DATA-DOG/go-sqlmock.v1/sqlmock.go:373 +0x19e\r\n  domain.example.com/group/project.TestprojectManager()\r\n      /u01/dev/go/src/domain.example.com/group/project/project_test.go:40 +0xba8\r\n  testing.tRunner()\r\n      /usr/local/go/src/testing/testing.go:610 +0xc9\r\n\r\nPrevious read at 0x00c420066e68 by goroutine 34:\r\n  domain.example.com/group/project/vendor/gopkg.in/DATA-DOG/go-sqlmock%2ev1.(*sqlmock).Query()\r\n      /u01/dev/go/src/domain.example.com/group/project/vendor/gopkg.in/DATA-DOG/go-sqlmock.v1/sqlmock.go:341 +0xaa5\r\n  database/sql.(*DB).queryConn()\r\n      /usr/local/go/src/database/sql/sql.go:1092 +0x68b\r\n  database/sql.(*DB).query()\r\n      /usr/local/go/src/database/sql/sql.go:1079 +0x188\r\n  database/sql.(*DB).Query()\r\n      /usr/local/go/src/database/sql/sql.go:1062 +0xad\r\n  database/sql.(*DB).QueryRow()\r\n      /usr/local/go/src/database/sql/sql.go:1143 +0x80\r\n  domain.example.com/group/project.(*channel).poll()\r\n      /u01/dev/go/src/domain.example.com/group/project/channel.go:84 +0x2e8\r\n  domain.example.com/group/project.(*channel).poller()\r\n      /u01/dev/go/src/domain.example.com/group/project/channel.go:58 +0x9e\r\n      \r\nGoroutine 32 (running) created at:\r\n  testing.(*T).Run()\r\n      /usr/local/go/src/testing/testing.go:646 +0x52f\r\n  testing.RunTests.func1()\r\n      /usr/local/go/src/testing/testing.go:793 +0xb9\r\n  testing.tRunner()\r\n      /usr/local/go/src/testing/testing.go:610 +0xc9\r\n  testing.RunTests()\r\n      /usr/local/go/src/testing/testing.go:799 +0x4ba\r\n  testing.(*M).Run()\r\n      /usr/local/go/src/testing/testing.go:743 +0x12f\r\n  main.main()\r\n      domain.example.com/group/project/_test/_testmain.go:78 +0x1b8\r\n\r\nGoroutine 34 (running) created at:\r\n  domain.example.com/group/project.newChannel()\r\n      /u01/dev/go/src/domain.example.com/group/project/channel.go:40 +0x1e0\r\n  domain.example.com/group/project.(*projectManager).add()\r\n      /u01/dev/go/src/domain.example.com/group/project/project.go:37 +0xca\r\n  domain.example.com/group/project.(*projectManager).pollDB()\r\n      /u01/dev/go/src/domain.example.com/group/project/project.go:87 +0x684\r\n  domain.example.com/group/project.NewprojectManager()\r\n      /u01/dev/go/src/domain.example.com/group/project/project.go:28 +0x11f\r\n  domain.example.com/group/project.TestprojectManager()\r\n      /u01/dev/go/src/domain.example.com/group/project/project_test.go:26 +0x70c\r\n  testing.tRunner()\r\n      /usr/local/go/src/testing/testing.go:610 +0xc9\r\n```\r\n\r\nEssentially:\r\n- The *sql.DB object is passed to a background thread (Goroutine 34) which polls continuously and rapidly (this is the Query call in the previous read section of the above race).\r\n- On line 40 of project_test.go (Goroutine 32), we call `mock.ExpectQuery(....)`\r\n\r\nI'm raising this issue as the README states `supports concurrency and multiple connections` and I can't easily work around this (having all Expect* calls before Goroutine 34 would be executed) without greatly limiting the effectiveness of the test.\r\n\r\nAny thoughts, it appears the `c.expected` may need to be guarded as well?"},"comment":{"url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/comments/264834136","html_url":"https://github.com/DATA-DOG/go-sqlmock/issues/61#issuecomment-264834136","issue_url":"https://api.github.com/repos/DATA-DOG/go-sqlmock/issues/61","id":264834136,"user":{"login":"l3pp4rd","id":132389,"avatar_url":"https://avatars.githubusercontent.com/u/132389?v=3","gravatar_id":"","url":"https://api.github.com/users/l3pp4rd","html_url":"https://github.com/l3pp4rd","followers_url":"https://api.github.com/users/l3pp4rd/followers","following_url":"https://api.github.com/users/l3pp4rd/following{/other_user}","gists_url":"https://api.github.com/users/l3pp4rd/gists{/gist_id}","starred_url":"https://api.github.com/users/l3pp4rd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/l3pp4rd/subscriptions","organizations_url":"https://api.github.com/users/l3pp4rd/orgs","repos_url":"https://api.github.com/users/l3pp4rd/repos","events_url":"https://api.github.com/users/l3pp4rd/events{/privacy}","received_events_url":"https://api.github.com/users/l3pp4rd/received_events","type":"User","site_admin":false},"created_at":"2016-12-05T11:44:05Z","updated_at":"2016-12-05T11:44:05Z","body":"I think the general issue is that, you define expectation after you trigger the query. that is where the race condition appears on regular expression parsing and comparison. This does not seem to be right from test case perspective, because:\r\n\r\n1. if sqlmock would allow to expect things concurrently together with running queries\r\n2. then the test cases would be false positive\r\n3. and this is not what you would expect right?\r\n\r\nIf we define what we expect, before running the queries, everything works as we would expect:\r\n\r\n``` go\r\n// TestIssue61ConcurrentExpectQuery ensures there's no race between\r\n// db.Query and mock.ExpectQuery when running with go test -race\r\nfunc TestIssue61ConcurrentExpectQuery(t *testing.T) {\r\n\tdb, mock, err := New()\r\n\tif err != nil {\r\n\t\tt.Fatalf(\"error creating mock database: %v\", err)\r\n\t}\r\n\tdefer db.Close()\r\n\r\n\t// Execute ExpectQuery, if race detector is enabled this will fail if there's a race\r\n\tmock.\r\n\t\tExpectQuery(\"SELECT (.+) FROM orders (.+) FOR UPDATE\").\r\n\t\tWillReturnRows(NewRows([]string{\"id\"}).AddRow(1))\r\n\r\n\t// Constantly call query in a background thread\r\n\tdone := make(chan bool)\r\n\tgo func() {\r\n\t\tfor {\r\n\t\t\tselect {\r\n\t\t\tcase <-done:\r\n\t\t\t\treturn\r\n\t\t\tdefault:\r\n\t\t\t\tdb.Query(\"SELECT id FROM orders WHERE id = 1 FOR UPDATE\")\r\n\t\t\t}\r\n\t\t}\r\n\t}()\r\n\r\n\tclose(done)\r\n}\r\n```\r\n\r\nThough, in this case the query does not get triggered. since channel is closed first.\r\n\r\nIf you add some time to run the select statement, the result still has no race conditions:\r\n\r\n``` go\r\n// TestIssue61ConcurrentExpectQuery ensures there's no race between\r\n// db.Query and mock.ExpectQuery when running with go test -race\r\nfunc TestIssue61ConcurrentExpectQuery(t *testing.T) {\r\n\tdb, mock, err := New()\r\n\tif err != nil {\r\n\t\tt.Fatalf(\"error creating mock database: %v\", err)\r\n\t}\r\n\tdefer db.Close()\r\n\r\n\t// Execute ExpectQuery, if race detector is enabled this will fail if there's a race\r\n\tmock.\r\n\t\tExpectQuery(\"SELECT (.+) FROM orders (.+) FOR UPDATE\").\r\n\t\tWillReturnRows(NewRows([]string{\"id\"}).AddRow(1))\r\n\r\n\t// Constantly call query in a background thread\r\n\tdone := make(chan bool)\r\n\tgo func() {\r\n\t\tfor {\r\n\t\t\tselect {\r\n\t\t\tcase <-done:\r\n\t\t\t\tfmt.Fprintln(os.Stderr, \"done\")\r\n\t\t\t\treturn\r\n\t\t\tdefault:\r\n\t\t\t\tdb.Query(\"SELECT id FROM orders WHERE id = 1 FOR UPDATE\")\r\n\t\t\t\tfmt.Fprintln(os.Stderr, \"triggered\")\r\n\t\t\t}\r\n\t\t}\r\n\t}()\r\n\r\n\ttime.Sleep(time.Millisecond * 50)\r\n\tclose(done)\r\n}\r\n```\r\n\r\nAnd you should see some triggers before channel is closed.\r\nAlso I would suggest instead of using **default** switch, to use `<-time.After(time.Millisecond * 10)` some small frequency checks\r\n\r\nSo to conclude, you should first expect something and not make a race condition for an expect and actual query. It is good that it fails, because it shows a problem in your test scenario.\r\n\r\nDoes it make sense?"}}
{"actor":{"display_login":"l3pp4rd"}}