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
fix(spanner): fix session leak #3461
Conversation
Thanks for the fix @8398a7. I would like @olavloite to approve this one. |
1785fac
to
0ad6950
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
spanner/client_test.go
Outdated
|
||
_, err := client.ReadWriteTransaction(ctx, func(ctx context.Context, tx *ReadWriteTransaction) error { | ||
return nil | ||
}) | ||
if err != nil { | ||
t.Fatalf("Unexpected error during transaction: %v", err) | ||
} |
There was a problem hiding this comment.
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.
_, 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) | |
} |
There was a problem hiding this comment.
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
@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. |
Kokoro only starts automatically for a "trusted" group. |
Ah. Thanks for finding that out. I guess that's reasonable. |
Fixed a problem where the session was not reused when a panic occurred.
Fixes #3460