fix(spanner): fix createMultiplexedSession goroutine leak #13396
Conversation
Summary of ChangesHello @egonelbre, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a goroutine leak within the Spanner client's session management. By explicitly closing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix a goroutine leak by closing the multiplexedSessionReq channel when the session pool is closed. While this correctly terminates the consumer goroutine, the placement of the close() call introduces a critical race condition. Closing the channel after releasing the mutex can lead to a panic if another goroutine attempts to send a request concurrently with the pool closure. I've recommended moving the close() call to be within the mutex-protected block to ensure atomicity and prevent this potential panic.
rahul2393
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
@olavloite / @sakthivelmanii need help in one more approval to merge this one, please help |
| } | ||
| p.mu.Unlock() | ||
| p.hc.close() | ||
| close(p.multiplexedSessionReq) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I created a draft for that test, however it fails at the moment #13406. Need to diagnose the issue, might require some additional fixes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
4 participants
No description provided.