Conversation
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.8.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:718167d5c23ed389b41f617b3a00ac839bdd938a6bd2d48ae0c2f1fa51ab1c3d <details><summary>storage: 1.59.2</summary> ## [1.59.2](storage/v1.59.1...storage/v1.59.2) (2026-01-28) ### Bug Fixes * deadlock in event loop while coordinating channels (#13652) ([ff6c8e7](ff6c8e78)) </details>
![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 updates the storage module to version 1.59.2 and introduces an internal request queue in the gRPC multi-range downloader to address a deadlock and prevent blocking the event loop. However, the implementation of the queue limit in the eventLoop introduces a potential deadlock scenario where the manager could become unresponsive if the queue fills up before a session is established. It is recommended to refine the command processing logic to ensure administrative commands can still be processed. Additionally, there are minor suggestions to correct a typo in the changelog date and simplify code in grpc_reader_multi_range.go by removing a redundant check.
| // Only read from cmds if we have space in the unsentRequests queue. | ||
| var cmdsChan chan mrdCommand | ||
| if m.unsentRequests.Len() < mrdAddInternalQueueMaxSize { | ||
| cmdsChan = m.cmds | ||
| } | ||
| select { | ||
| case <-m.ctx.Done(): | ||
| return | ||
| case cmd := <-m.cmds: | ||
| // This path only triggers if space is available in the channel. | ||
| // It never blocks the eventLoop. | ||
| case targetChan <- nextReq: | ||
| m.unsentRequests.RemoveFront() | ||
| case cmd := <-cmdsChan: | ||
| cmd.apply(m.ctx, m) | ||
| if _, ok := cmd.(*mrdCloseCmd); ok { | ||
| return |
There was a problem hiding this comment.
The eventLoop implementation contains a logic flaw that can lead to a permanent deadlock. When the unsentRequests queue reaches its maximum capacity (mrdAddInternalQueueMaxSize = 50000), the cmdsChan is set to nil (line 397). If at the same time m.currentSession is nil (e.g., at startup or after a session failure), the targetChan also remains nil (line 385). In Go, a select statement with only nil channels (except for the context's Done() channel) will block indefinitely. This prevents the eventLoop from processing any further commands, including those that would initialize a session to drain the queue or even a command to close the downloader. This results in a Denial of Service for the MultiRangeDownloader component.
| # Changes | ||
|
|
||
|
|
||
| ## [1.59.2](https://github.com/googleapis/google-cloud-go/releases/tag/storage%2Fv1.59.2) (2026-01-28) |
There was a problem hiding this comment.
The date (2026-01-28) appears to be a typo, as it is several years in the future. Please correct the year.
| if m.unsentRequests.Len() > 0 && m.currentSession != nil { | ||
| nextReq = m.unsentRequests.Front() | ||
| if nextReq != nil { | ||
| targetChan = m.currentSession.reqC | ||
| } | ||
| } |
There was a problem hiding this comment.
The check if nextReq != nil is redundant. If m.unsentRequests.Len() > 0, then m.unsentRequests.Front() is guaranteed not to return nil. You can simplify this block for better clarity.
if m.unsentRequests.Len() > 0 && m.currentSession != nil {
nextReq = m.unsentRequests.Front()
targetChan = m.currentSession.reqC
}1 participant
No description provided.