fix(storage): close HTTP2 response bodies to prevent flow-control leaks#14324
fix(storage): close HTTP2 response bodies to prevent flow-control leaks#14324krishnamd-jkp wants to merge 2 commits intogoogleapis:mainfrom
Conversation
Fixes an issue where HTTP2 stream connections might exhaust flow-control quota and cause INTERNAL_ERROR in long-lived connections. - Modifies `httpReader.Read` to properly close the previous stream body before reopening it. - Adds a defer block in `parseReadResponse` to ensure response bodies aren't leaked when an error is returned during header parsing. - Ensures bodies are closed when stream discarding (`io.CopyN`) or `X-Goog-Generation` header parsing fails in `readerReopen`. These fixes apply equally to both the JSON and XML reader code paths. Co-authored-by: krishnamd-jkp <230955344+krishnamd-jkp@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request improves resource management in storage/http_client.go by ensuring http.Response.Body is consistently closed during error conditions and before retrying read operations. Key changes include moving the body closure before reopening connections in the Read method and implementing a deferred closure in parseReadResponse using named return parameters. Feedback suggests enhancing error context in parseReadResponse by wrapping the strconv.ParseInt error when handling invalid Content-Range headers.
| err = fmt.Errorf("storage: invalid Content-Range %q", cr) | ||
| return nil, err |
There was a problem hiding this comment.
The error from strconv.ParseInt is discarded here. Per repository rules, helper functions that parse strings should include error handling for parsing failures. For consistency and better error context, consider wrapping the error here as well.
| err = fmt.Errorf("storage: invalid Content-Range %q", cr) | |
| return nil, err | |
| err = fmt.Errorf("storage: invalid Content-Range %q: %w", cr, err) | |
| return nil, err |
References
- When implementing helper functions that parse strings, always include error handling for parsing failures.
Fixes an issue where HTTP2 stream connections might exhaust flow-control quota and cause INTERNAL_ERROR in long-lived connections.
httpReader.Readto properly close the previous stream body before reopening it.parseReadResponseto ensure response bodies aren't leaked when an error is returned during header parsing.io.CopyN) orX-Goog-Generationheader parsing fails inreaderReopen. These fixes apply equally to both the JSON and XML reader code paths.