Skip to content

fix(spanner): fix createMultiplexedSession goroutine leak #13396

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

Merged
rahul2393 merged 1 commit into from
Nov 25, 2025
Merged

fix(spanner): fix createMultiplexedSession goroutine leak #13396

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spanner/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,7 @@ func (p *sessionPool) close(ctx context.Context) {
}
p.mu.Unlock()
p.hc.close()
close(p.multiplexedSessionReq)
Comment thread
egonelbre marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@olavloite olavloite Nov 25, 2025

Choose a reason for hiding this comment

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

This change would get lost if we remove the entire sessionPool implementation (which is the intention). So it might be better to put this line of code in the Client#Close() function instead to make sure it is not unintentionally deleted. AFAICT, there are no tests that verify that this is actually closed, meaning that it would also not be caught by any tests.

Copy link
Copy Markdown
Contributor Author

@egonelbre egonelbre Nov 25, 2025

Choose a reason for hiding this comment

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

I think the property that should be enforced is that all started goroutines are stopped. If the channels is left open and no one is using it, then it probably doesn't matter.

Using synctest seems a good fit for this. However, it does require Go 1.25 or 1.24 with GOEXPERIMENT=synctest. I wouldn't want to bump minimum language version requirement just for that... however the test could be put behind build tag.

Copy link
Copy Markdown
Contributor Author

@egonelbre egonelbre Nov 25, 2025

Choose a reason for hiding this comment

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

I created a draft for that test, however it fails at the moment #13406. Need to diagnose the issue, might require some additional fixes.

Copy link
Copy Markdown
Contributor

@olavloite olavloite Nov 25, 2025

Choose a reason for hiding this comment

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

SG. I didn't really realize that the entire multiplexed session implementation lives in the session pool, so my comment that this call should be moved to outside the pool does not really make any sense.

Copy link
Copy Markdown
Contributor Author

@egonelbre egonelbre Nov 25, 2025

Choose a reason for hiding this comment

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

I managed to fix #13406 and it succeeds when this PR is merged. However, the mock server still leaks, which I excluded from the check.

// destroy all the sessions
p.hc.mu.Lock()
allSessions := make([]*session, len(p.hc.queue.sessions))
Expand Down
Loading