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

fix(spanner): fix session leak #3461

Merged
merged 4 commits into from Dec 16, 2020
Merged

fix(spanner): fix session leak #3461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ad6950
fix(spanner): fix session leak
8398a7 Dec 15, 2020
df60414
Fix the test not to hang when reproducing
8398a7 Dec 16, 2020
dbd337d
Merge branch 'master' into fix-session-leak
olavloite Dec 16, 2020
8018137
Merge branch 'master' into fix-session-leak
olavloite Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions spanner/client.go
View file
Open in desktop
Expand Up @@ -451,6 +451,11 @@ func (c *Client) rwTransaction(ctx context.Context, f func(context.Context, *Rea
var (
sh *sessionHandle
)
defer func() {
if sh != nil {
sh.recycle()
}
}()
err = runWithRetryOnAbortedOrSessionNotFound(ctx, func(ctx context.Context) error {
var (
err error
Expand Down Expand Up @@ -480,9 +485,6 @@ func (c *Client) rwTransaction(ctx context.Context, f func(context.Context, *Rea
resp, err = t.runInTransaction(ctx, f)
return err
})
if sh != nil {
sh.recycle()
}
return resp, err
}

Expand Down
31 changes: 31 additions & 0 deletions spanner/client_test.go
View file
Open in desktop
Expand Up @@ -828,6 +828,37 @@ func TestClient_ReadWriteTransaction_Update_QueryOptions(t *testing.T) {
}
}

func TestClient_ReadWriteTransaction_DoNotLeakSessionOnPanic(t *testing.T) {
// Make sure that there is always only one session in the pool.
sc := SessionPoolConfig{
MinOpened: 1,
MaxOpened: 1,
}
_, client, teardown := setupMockedTestServerWithConfig(t, ClientConfig{SessionPoolConfig: sc})
defer teardown()
ctx := context.Background()

// If a panic occurs during a transaction, the session will not leak.
func() {
defer func() { recover() }()

_, err := client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *ReadWriteTransaction) error {
panic("cause panic")
return nil
})
if err != nil {
t.Fatalf("Unexpected error during transaction: %v", err)
}
}()

_, err := client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *ReadWriteTransaction) error {
return nil
})
if err != nil {
t.Fatalf("Unexpected error during transaction: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current test case will hang indefinitely if a session leak would be re-introduced by accident in the future. The below suggestion would cause the test to fail fast instead.

Suggested change
_, err := client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *ReadWriteTransaction) error {
return nil
})
if err != nil {
t.Fatalf("Unexpected error during transaction: %v", err)
}
if g, w := client.idleSessions.idleList.Len(), 1; g != w {
t.Fatalf("idle session count mismatch.\nGot: %v\nWant: %v", g, w)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely a problem when it hangs.

Fixed.
df60414

}

func TestClient_SessionNotFound(t *testing.T) {
// Ensure we always have at least one session in the pool.
sc := SessionPoolConfig{
Expand Down