Skip to content

Commit 44175c5

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 44175c5

12 files changed

+124
-43
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: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ func runAndAssert(suite suite.Suite, expected, v interface{}, session *r.Session
5353
}
5454

5555
assertExpected(suite, expected, cursor, err)
56+
57+
if cursor != nil {
58+
cursor.Close()
59+
}
5660
}
5761

5862
func fetchAndAssert(suite suite.Suite, expected, result interface{}, count int) {
@@ -81,6 +85,16 @@ func fetchAndAssert(suite suite.Suite, expected, result interface{}, count int)
8185
}
8286

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

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

‎internal/integration/reql_tests/reql_changefeeds_edge_test.go

Lines changed: 8 additions & 0 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: 8 additions & 0 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: 8 additions & 0 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: 8 additions & 0 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: 8 additions & 0 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: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,17 @@ func (suite *${module_name}Suite) TearDownSuite() {
5151
suite.T().Log("Tearing down ${module_name}Suite")
5252
5353
if suite.session != nil {
54+
/* TearDownSuite() will block if a cursor is left open by a test, because:
55+
* the pool only contains one connection by default,
56+
* that connection can only be used by the goroutine that acquired it (until it's released), and
57+
* stretchr/testify/suite runs SetupTest() and TestCases() in a different goroutine than TearDownSuite().
58+
*/
59+
err := suite.session.Reconnect()
60+
suite.Require().NoError(err, "Error returned when reconnecting to server")
61+
5462
r.DB("rethinkdb").Table("_debug_scratch").Delete().Exec(suite.session)
5563
%for var_name in table_var_names:
56-
r.DB("test").TableDrop("${var_name}").Exec(suite.session)
64+
r.DB("test").TableDrop("${var_name}").Exec(suite.session)
5765
%endfor
5866
r.DBDrop("test").Exec(suite.session)
5967

‎internal/integration/tests/cursor_test.go

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package tests
22

33
import (
4-
"fmt"
54
"time"
65

76
test "gopkg.in/check.v1"
@@ -195,6 +194,8 @@ func (s *RethinkSuite) TestEmptyResults(c *test.C) {
195194
res, err := r.DB("test_empty_res").Table("test_empty_res").Get("missing value").Run(session)
196195
c.Assert(err, test.IsNil)
197196
c.Assert(res.IsNil(), test.Equals, true)
197+
err = res.Close()
198+
c.Assert(err, test.IsNil)
198199

199200
res, err = r.DB("test_empty_res").Table("test_empty_res").Get("missing value").Run(session)
200201
c.Assert(err, test.IsNil)
@@ -206,27 +207,37 @@ func (s *RethinkSuite) TestEmptyResults(c *test.C) {
206207
res, err = r.Expr(nil).Run(session)
207208
c.Assert(err, test.IsNil)
208209
c.Assert(res.IsNil(), test.Equals, true)
210+
err = res.Close()
211+
c.Assert(err, test.IsNil)
209212

210213
res, err = r.DB("test_empty_res").Table("test_empty_res").Get("missing value").Run(session)
211214
c.Assert(err, test.IsNil)
212215
c.Assert(res.IsNil(), test.Equals, true)
216+
err = res.Close()
217+
c.Assert(err, test.IsNil)
213218

214219
res, err = r.DB("test_empty_res").Table("test_empty_res").GetAll("missing value", "another missing value").Run(session)
215220
c.Assert(err, test.IsNil)
216221
c.Assert(res.Next(&response), test.Equals, false)
222+
err = res.Close()
223+
c.Assert(err, test.IsNil)
217224

218225
var obj object
219226
obj.Name = "missing value"
220227
res, err = r.DB("test_empty_res").Table("test_empty_res").Filter(obj).Run(session)
221228
c.Assert(err, test.IsNil)
222229
c.Assert(res.IsNil(), test.Equals, true)
230+
err = res.Close()
231+
c.Assert(err, test.IsNil)
223232

224233
var objP *object
225234

226235
res, err = r.DB("test_empty_res").Table("test_empty_res").Get("missing value").Run(session)
227236
res.Next(&objP)
228237
c.Assert(err, test.IsNil)
229238
c.Assert(objP, test.IsNil)
239+
err = res.Close()
240+
c.Assert(err, test.IsNil)
230241
}
231242

232243
func (s *RethinkSuite) TestCursorAll(c *test.C) {
@@ -346,6 +357,9 @@ func (s *RethinkSuite) TestCursorListen(c *test.C) {
346357
}},
347358
},
348359
})
360+
361+
err = res.Close()
362+
c.Assert(err, test.IsNil)
349363
}
350364

351365
func (s *RethinkSuite) TestCursorChangesClose(c *test.C) {
@@ -424,6 +438,9 @@ func (s *RethinkSuite) TestCursorReuseResult(c *test.C) {
424438
i++
425439
}
426440
c.Assert(res.Err(), test.IsNil)
441+
442+
err = res.Close()
443+
c.Assert(err, test.IsNil)
427444
}
428445

429446
func (s *RethinkSuite) TestCursorNextResponse(c *test.C) {
@@ -434,6 +451,9 @@ func (s *RethinkSuite) TestCursorNextResponse(c *test.C) {
434451
b, ok := res.NextResponse()
435452
c.Assert(ok, test.Equals, true)
436453
c.Assert(b, JsonEquals, []byte(`5`))
454+
455+
err = res.Close()
456+
c.Assert(err, test.IsNil)
437457
}
438458

439459
func (s *RethinkSuite) TestCursorNextResponse_object(c *test.C) {
@@ -444,6 +464,9 @@ func (s *RethinkSuite) TestCursorNextResponse_object(c *test.C) {
444464
b, ok := res.NextResponse()
445465
c.Assert(ok, test.Equals, true)
446466
c.Assert(b, JsonEquals, []byte(`{"foo":"bar"}`))
467+
468+
err = res.Close()
469+
c.Assert(err, test.IsNil)
447470
}
448471

449472
func (s *RethinkSuite) TestCursorPeek_idempotency(c *test.C) {
@@ -460,6 +483,8 @@ func (s *RethinkSuite) TestCursorPeek_idempotency(c *test.C) {
460483
c.Assert(hasMore, test.Equals, true)
461484
}
462485

486+
err = res.Close()
487+
c.Assert(err, test.IsNil)
463488
}
464489

465490
func (s *RethinkSuite) TestCursorPeek_wrong_type(c *test.C) {
@@ -476,6 +501,9 @@ func (s *RethinkSuite) TestCursorPeek_wrong_type(c *test.C) {
476501
c.Assert(err, test.NotNil)
477502
c.Assert(hasMore, test.Equals, false)
478503
c.Assert(res.Err(), test.IsNil)
504+
505+
err = res.Close()
506+
c.Assert(err, test.IsNil)
479507
}
480508

481509
func (s *RethinkSuite) TestCursorPeek_usage(c *test.C) {
@@ -495,6 +523,9 @@ func (s *RethinkSuite) TestCursorPeek_usage(c *test.C) {
495523
hasMore = res.Next(&result)
496524
c.Assert(result, test.Equals, 2)
497525
c.Assert(hasMore, test.Equals, true)
526+
527+
err = res.Close()
528+
c.Assert(err, test.IsNil)
498529
}
499530

500531
func (s *RethinkSuite) TestCursorSkip(c *test.C) {
@@ -507,26 +538,7 @@ func (s *RethinkSuite) TestCursorSkip(c *test.C) {
507538
hasMore := res.Next(&result)
508539
c.Assert(result, test.Equals, 2)
509540
c.Assert(hasMore, test.Equals, true)
510-
}
511-
512-
func ExampleCursor_Peek() {
513-
res, err := r.Expr([]int{1, 2, 3}).Run(session)
514-
if err != nil {
515-
fmt.Print(err)
516-
return
517-
}
518541

519-
var result, altResult int
520-
wasRead, err := res.Peek(&result) // Result is now 1
521-
if err != nil {
522-
fmt.Print(err)
523-
return
524-
} else if !wasRead {
525-
fmt.Print("No data to read!")
526-
}
527-
528-
res.Next(&altResult) // altResult is also 1, peek didn't progress the cursor
529-
530-
res.Skip() // progress the cursor, skipping 2
531-
res.Peek(&result) // result is now 3
542+
err = res.Close()
543+
c.Assert(err, test.IsNil)
532544
}

‎internal/integration/tests/example_query_table_test.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ package tests
22

33
import (
44
"fmt"
5+
56
r "gopkg.in/rethinkdb/rethinkdb-go.v6"
67
)
78

89
// Create a table named "table" with the default settings.
910
func ExampleTerm_TableCreate() {
1011
// Setup database
11-
r.DB("examples").TableDrop("table").Run(session)
12+
r.DB("examples").TableDrop("table").Exec(session)
1213

1314
response, err := r.DB("examples").TableCreate("table").RunWrite(session)
1415
if err != nil {
@@ -24,8 +25,8 @@ func ExampleTerm_TableCreate() {
2425
// Create a simple index based on the field name.
2526
func ExampleTerm_IndexCreate() {
2627
// Setup database
27-
r.DB("examples").TableDrop("table").Run(session)
28-
r.DB("examples").TableCreate("table").Run(session)
28+
r.DB("examples").TableDrop("table").Exec(session)
29+
r.DB("examples").TableCreate("table").Exec(session)
2930

3031
response, err := r.DB("examples").Table("table").IndexCreate("name").RunWrite(session)
3132
if err != nil {
@@ -41,8 +42,8 @@ func ExampleTerm_IndexCreate() {
4142
// Create a compound index based on the fields first_name and last_name.
4243
func ExampleTerm_IndexCreate_compound() {
4344
// Setup database
44-
r.DB("examples").TableDrop("table").Run(session)
45-
r.DB("examples").TableCreate("table").Run(session)
45+
r.DB("examples").TableDrop("table").Exec(session)
46+
r.DB("examples").TableCreate("table").Exec(session)
4647

4748
response, err := r.DB("examples").Table("table").IndexCreateFunc("full_name", func(row r.Term) interface{} {
4849
return []interface{}{row.Field("first_name"), row.Field("last_name")}

‎internal/integration/tests/query_test.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,9 @@ func (s *RethinkSuite) TestTableChanges(c *test.C) {
485485
wg.Wait()
486486

487487
c.Assert(n, test.Equals, 10)
488+
489+
err = res.Close()
490+
c.Assert(err, test.IsNil)
488491
}
489492

490493
func (s *RethinkSuite) TestTableChangesExit(c *test.C) {
@@ -599,6 +602,9 @@ func (s *RethinkSuite) TestTableChangesIncludeInitial(c *test.C) {
599602
wg.Wait()
600603

601604
c.Assert(n, test.Equals, 10)
605+
606+
err = res.Close()
607+
c.Assert(err, test.IsNil)
602608
}
603609

604610
func (s *RethinkSuite) TestWriteReference(c *test.C) {
@@ -773,5 +779,6 @@ func (s *RethinkSuite) TestSelectManyRows(c *test.C) {
773779
c.Assert(res.Err(), test.IsNil)
774780
c.Assert(n, test.Equals, 10000)
775781

776-
res.Close()
782+
err = res.Close()
783+
c.Assert(err, test.IsNil)
777784
}

0 commit comments

Comments
 (0)