fix(storage): handle MRD hang corner case#14509
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a managerCtx to the bidiReadStreamSession to improve lifecycle management and prevent race conditions when sending results back to the manager. Key changes include refined error handling in the send and receive loops and explicit checks for context cancellation. Feedback was provided to ensure that databufs are consistently freed in all error paths of the receiveLoop to prevent memory leaks and to further guard against sending on closed channels.
| if s.managerCtx.Err() != nil { | ||
| return | ||
| } | ||
| select { | ||
| case s.respC <- result: | ||
| case <-s.ctx.Done(): | ||
| case <-s.managerCtx.Done(): | ||
| } | ||
| return |
There was a problem hiding this comment.
In the error handling path of receiveLoop, databufs should be freed to avoid memory leaks. Additionally, to prevent sending on a closed channel (if the cancellation cleanup logic closes s.respC), explicitly check for context cancellation before the select block. This ensures that we do not attempt to send on s.respC if the context is already canceled, aligning with repository safety patterns.
if s.managerCtx.Err() != nil {
databufs.Free()
return
}
select {
case s.respC <- result:
case <-s.managerCtx.Done():
}
databufs.Free()
returnReferences
- To prevent sending on a closed channel, explicitly check for context cancellation before a select block that both sends on that channel and checks for the context's cancellation. This is necessary when the cancellation cleanup logic is responsible for closing the channel.
| } | ||
| s.setError(err) | ||
|
|
||
| if s.managerCtx.Err() != nil { |
There was a problem hiding this comment.
isn't this condition covered in the below select?
Same with line 904.
There was a problem hiding this comment.
This is in case of error. Once inside this if condition, the below block is not triggered.
There was a problem hiding this comment.
If we are inside the switch statement when the error is thrown, could there still be a hang, this reduces the probability of it?
There was a problem hiding this comment.
Oh okay. Misread it. So I'm making sure if manager context is done, I don't still accidentally select the case s.respC <- result: case because that would lead to a panic if sent on a closed channel
b3112f4
into
googleapis:release-storage-1.59.2
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.12.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:718167d5c23ed389b41f617b3a00ac839bdd938a6bd2d48ae0c2f1fa51ab1c3d <details><summary>storage: v1.59.3</summary> ## [v1.59.3](storage/v1.59.2...storage/v1.59.3) (2026-05-05) ### Bug Fixes * handle MRD hang corner case (#14509) ([1ca3b6f](1ca3b6f0)) </details>
No description provided.