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.

Sign up for GitHub

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

Jump to bottom

Support broken database cluster on startup #6

Closed
wants to merge 1 commit into from
Closed

Support broken database cluster on startup #6

wants to merge 1 commit into from

Conversation

bbrodriges
Copy link
Contributor

Error pops up while running newly added test with -race flag

$ go test -v -race -run=^TestCluster_BrokenAtStartup$ ./...
You use x86_64 version of selected tool.
=== RUN   TestCluster_BrokenAtStartup
=== RUN   TestCluster_BrokenAtStartup/broken_primary
==================
WARNING: DATA RACE
Read at 0x00c0001901d8 by goroutine 21:
  github.com/DATA-DOG/go-sqlmock.(*rowSets).Next()
      /Users/gzuykov/go/pkg/mod/github.com/!d!a!t!a-!d!o!g/[email protected]/rows.go:45 +0xa8
  database/sql.(*Rows).nextLocked()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2967 +0x1d0
  database/sql.(*Rows).Next.func1()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2945 +0x48
  database/sql.withLock()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3396 +0x74
  database/sql.(*Rows).Next()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2944 +0x7c
  database/sql.(*Row).Scan()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3333 +0x108
  golang.yandex/hasql/checkers.Check()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/check.go:29 +0xd8
  golang.yandex/hasql/checkers.PostgreSQL()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/postgresql.go:26 +0x54
  golang.yandex/hasql.checkExecutor.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:149 +0x80
  golang.yandex/hasql.checkNodes.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:109 +0x88

Previous write at 0x00c0001901d8 by goroutine 20:
  github.com/DATA-DOG/go-sqlmock.(*rowSets).Next()
      /Users/gzuykov/go/pkg/mod/github.com/!d!a!t!a-!d!o!g/[email protected]/rows.go:45 +0xc0
  database/sql.(*Rows).nextLocked()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2967 +0x1d0
  database/sql.(*Rows).Next.func1()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2945 +0x48
  database/sql.withLock()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3396 +0x74
  database/sql.(*Rows).Next()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2944 +0x7c
  database/sql.(*Row).Scan()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3333 +0x108
  golang.yandex/hasql/checkers.Check()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/check.go:29 +0xd8
  golang.yandex/hasql/checkers.PostgreSQL()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/postgresql.go:26 +0x54
  golang.yandex/hasql.checkExecutor.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:149 +0x80
  golang.yandex/hasql.checkNodes.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:109 +0x88

Goroutine 21 (running) created at:
  golang.yandex/hasql.checkNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:106 +0x450
  golang.yandex/hasql.(*Cluster).updateNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:321 +0x1b4
  golang.yandex/hasql.(*Cluster).backgroundNodesUpdate()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:297 +0x30

diff --git a/cluster_test.go b/cluster_test.go
index 10c22eb..c816f2f 100644
--- a/cluster_test.go
+++ b/cluster_test.go
@@ -29,6 +29,8 @@ import (
        "github.com/gofrs/uuid"
        "github.com/stretchr/testify/assert"
        "github.com/stretchr/testify/require"
+
+       "golang.yandex/hasql/checkers"
 )

 func TestNewCluster(t *testing.T) {
@@ -543,6 +545,89 @@ func TestCluster_WaitForStandbyPreferred(t *testing.T) {
        }
 }

+// TestCluster_BrokenAtStartup checks if broken at startup cluster behaves properly
+func TestCluster_BrokenAtStartup(t *testing.T) {
+       isPrimaryRow := sqlmock.NewRows([]string{`NOT pg_is_in_recovery()`}).AddRow(true)
+       isStandbyRow := sqlmock.NewRows([]string{`NOT pg_is_in_recovery()`}).AddRow(false)
+
+       testCases := []struct {
+               name  string
+               nodes []Node
+       }{
+               {
+                       name: "broken_primary",
+                       nodes: func() []Node {
+                               db1, mock1, _ := sqlmock.New() // primary
+                               db2, mock2, _ := sqlmock.New() // standby
+                               db3, mock3, _ := sqlmock.New() // standby
+
+                               mock1.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillDelayFor(20 * time.Millisecond)
+                               mock2.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillReturnRows(isStandbyRow)
+                               mock3.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillReturnRows(isStandbyRow)
+
+                               return []Node{
+                                       NewNode("primary", db1),
+                                       NewNode("standby1", db2),
+                                       NewNode("standby2", db3),
+                               }
+                       }(),
+               },
+               {
Goroutine 20 (finished) created at:
  golang.yandex/hasql.checkNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:106 +0x450
  golang.yandex/hasql.(*Cluster).updateNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:321 +0x1b4
  golang.yandex/hasql.(*Cluster).backgroundNodesUpdate()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:297 +0x30
==================
    testing.go:1152: race detected during execution of test
=== RUN   TestCluster_BrokenAtStartup/broken_standby
=== RUN   TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:624:
        	Error Trace:	cluster_test.go:624
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:625:
        	Error Trace:	cluster_test.go:625
        	Error:      	Expected value not to be nil.
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:626:
        	Error Trace:	cluster_test.go:626
        	Error:      	Should NOT be empty, but was []
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
=== CONT  TestCluster_BrokenAtStartup
    testing.go:1152: race detected during execution of test
--- FAIL: TestCluster_BrokenAtStartup (0.01s)
    --- FAIL: TestCluster_BrokenAtStartup/broken_primary (0.00s)
    --- PASS: TestCluster_BrokenAtStartup/broken_standby (0.00s)
    --- FAIL: TestCluster_BrokenAtStartup/broken_standbys (0.01s)
=== CONT
    testing.go:1152: race detected during execution of test
FAIL
FAIL	golang.yandex/hasql	0.286s
?   	golang.yandex/hasql/checkers	[no test files]
testing: warning: no tests to run
PASS
ok  	golang.yandex/hasql/sqlx	(cached) [no tests to run]
FAIL

@bbrodriges bbrodriges requested a review from sidh March 5, 2022 09:36
@sidh
Copy link
Collaborator

sidh commented Jun 9, 2022

Race is triggered because sqlmock.NewRows is not thread-safe. You must create one for each WillReturnRows.

I will look into the issue further.

@bbrodriges bbrodriges closed this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidh sidh Awaiting requested review from sidh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bbrodriges @sidh