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.

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

fix(spanner): fix session leak #3461

Merged
merged 4 commits into from Dec 16, 2020

Conversation

8398a7
Copy link
Contributor

@8398a7 8398a7 commented Dec 14, 2020

Fixed a problem where the session was not reused when a panic occurred.

Fixes #3460

@8398a7 8398a7 requested review from skuruppu and a team as code owners Dec 14, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 14, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Dec 14, 2020
@skuruppu skuruppu requested a review from olavloite Dec 15, 2020
@skuruppu
Copy link
Contributor

skuruppu commented Dec 15, 2020

Thanks for the fix @8398a7. I would like @olavloite to approve this one.

Copy link
Contributor

@olavloite olavloite left a comment

Thanks, that's a good catch!

Would you mind changing the test case so that it does not hang indefinitely if the problem were to reappear? I've added a suggestion, but feel free to do it in another way if you have a better idea.


_, 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

@olavloite olavloite Dec 16, 2020

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

@8398a7 8398a7 Dec 16, 2020

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

@olavloite
Copy link
Contributor

olavloite commented Dec 16, 2020

@tbpg This one also seems to be stuck waiting for kokoro to report the build status. Could it be that this is related to the fact that the PR is created from a fork? Other PRs do not seem to have this problem, AFAICT.

@tbpg tbpg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 16, 2020
@tbpg
Copy link
Contributor

tbpg commented Dec 16, 2020

Kokoro only starts automatically for a "trusted" group.

@olavloite
Copy link
Contributor

olavloite commented Dec 16, 2020

Kokoro only starts automatically for a "trusted" group.

Ah. Thanks for finding that out. I guess that's reasonable.

@olavloite olavloite added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 16, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 11fb917 into googleapis:master Dec 16, 2020
2 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 16, 2020
@8398a7 8398a7 deleted the fix-session-leak branch Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: If panic occurs in ReadWriteTransaction, the session will leak
5 participants