Skip to content

Commit 85005ba

Browse files
author
Cody Jones
committed
Fix a hang caused by bad interaction between new pool and testify
At present only the generated changefeed tests are affected, and running gen_tests produces many distracting changes, so I manually altered TearDownSuite() in only the changefeed tests. (Ultimately I'd like to see #495 and #496 merged, after which gen_tests can be run to fix TearDownSuite() in the other files for consistency.) This change also closes some cursors that were previously leaked in hand-written tests. In most cases this isn't necessary. But like the changefeed tests, sometimes the test framework uses multiple goroutines, in which case the default single-connection pool will block if any tests fail to close their cursors. Finally, this includes a .travis.yml change previously submitted in #494 so the build can be verified.
1 parent b8acfce commit 85005ba

13 files changed

+204
-71
lines changed

.travis.yml

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,9 @@ go:
77

88
go_import_path: gopkg.in/rethinkdb/rethinkdb-go.v6
99

10-
install: go get -t ./...
11-
1210
before_script:
13-
- source /etc/lsb-release && echo "deb http://download.rethinkdb.com/apt $DISTRIB_CODENAME main" | sudo tee /etc/apt/sources.list.d/rethinkdb.list
14-
- wget -qO- http://download.rethinkdb.com/apt/pubkey.gpg | sudo apt-key add -
11+
- source /etc/lsb-release && echo "deb https://download.rethinkdb.com/repository/ubuntu-$TRAVIS_DIST $TRAVIS_DIST main" | sudo tee /etc/apt/sources.list.d/rethinkdb.list
12+
- wget -qO- https://download.rethinkdb.com/repository/raw/pubkey.gpg | sudo apt-key add -
1513
- sudo apt-get update
1614
- sudo apt-get install rethinkdb
1715
- rethinkdb > /dev/null 2>&1 &
@@ -20,6 +18,7 @@ before_script:
2018
- rethinkdb --port-offset 3 --directory rethinkdb_data3 --join localhost:29016 > /dev/null 2>&1 &
2119

2220
script:
23-
- go test -race .
24-
- go test -tags='cluster' -short -race -v ./...
25-
- GOMODULE111=off go test .
21+
- GO111MODULE=on go test -race .
22+
- GO111MODULE=on go test -tags='cluster' -short -race -v ./...
23+
- GO111MODULE=on go test .
24+

internal/integration/reql_tests/common.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ func runAndAssert(suite suite.Suite, expected, v interface{}, session *r.Session
5353
}
5454

5555
assertExpected(suite, expected, cursor, err)
56+
57+
closeCursor(cursor)
5658
}
5759

5860
func fetchAndAssert(suite suite.Suite, expected, result interface{}, count int) {
@@ -81,6 +83,20 @@ func fetchAndAssert(suite suite.Suite, expected, result interface{}, count int)
8183
}
8284

8385
assertExpected(suite, expected, cursor, err)
86+
87+
closeCursor(cursor)
88+
}
89+
90+
func closeCursor(c *r.Cursor) {
91+
if c != nil {
92+
/* Cursors should ordinarily be closed when they are no longer needed, otherwise an application using
93+
multiple goroutines might hang waiting for a free connection in the pool. However, some of the
94+
generated changefeed tests expect the cursor will return additional changes after fetchAndAssert()
95+
is called. This means we must leave test cursors hanging open for these tests to pass.
96+
*/
97+
98+
//c.Close()
99+
}
84100
}
85101

86102
func maybeLen(v interface{}) interface{} {

internal/integration/reql_tests/reql_changefeeds_edge_test.go

Lines changed: 18 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/reql_changefeeds_idxcopy_test.go

Lines changed: 18 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/reql_changefeeds_include_states_test.go

Lines changed: 18 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/reql_changefeeds_point_test.go

Lines changed: 18 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/reql_changefeeds_table_test.go

Lines changed: 18 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/reql_datum_array_test.go

Lines changed: 17 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/integration/reql_tests/template.go.tpl

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,29 @@ func (suite *${module_name}Suite) SetupTest() {
5050
func (suite *${module_name}Suite) TearDownSuite() {
5151
suite.T().Log("Tearing down ${module_name}Suite")
5252
53-
if suite.session != nil {
54-
r.DB("rethinkdb").Table("_debug_scratch").Delete().Exec(suite.session)
55-
%for var_name in table_var_names:
56-
r.DB("test").TableDrop("${var_name}").Exec(suite.session)
57-
%endfor
58-
r.DBDrop("test").Exec(suite.session)
53+
/* This function will block if a cursor is left open by a test, because:
54+
* the pool only contains one connection by default,
55+
* that connection can only be used by the goroutine that acquired it (until it's released), and
56+
* stretchr/testify/suite runs SetupTest() and TestCases() in a different goroutine.
5957
58+
It's easiest to circumvent this and use a new session rather than altering the core Rethink tests or the Go test generator.
59+
*/
60+
if suite.session != nil {
6061
suite.session.Close()
6162
}
63+
64+
session, err := r.Connect(r.ConnectOpts{
65+
Address: url,
66+
})
67+
suite.Require().NoError(err, "Error returned when connecting to server")
68+
69+
r.DB("rethinkdb").Table("_debug_scratch").Delete().Exec(session)
70+
%for var_name in table_var_names:
71+
r.DB("test").TableDrop("${var_name}").Exec(session)
72+
%endfor
73+
r.DBDrop("test").Exec(session)
74+
75+
session.Close()
6276
}
6377

6478
<%rendered_vars = set() %>\

0 commit comments

Comments
 (0)