fix(backend): close the ReadCloser returned by PullBlob in push#509
Open
SAY-5 wants to merge 1 commit intomodelpack:mainfrom
Open
fix(backend): close the ReadCloser returned by PullBlob in push#509SAY-5 wants to merge 1 commit intomodelpack:mainfrom
SAY-5 wants to merge 1 commit intomodelpack:mainfrom
Conversation
pushIfNotExist pulled each blob's content from the source storage and then handed it to Blobs().Push wrapped in io.NopCloser. The NopCloser is there on purpose (Close on the distribution reader returns an error, see modelpack#50), but it also means the original ReadCloser from PullBlob was never closed on any path, leaking a file descriptor or HTTP body per blob. Add a `defer content.Close()` immediately after the nil-error check so the original reader is released on both success and error paths. The existing NopCloser wrapper still prevents Push from calling Close itself, so the workaround for modelpack#50 is preserved. Fixes modelpack#491 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request addresses a resource leak in pkg/backend/push.go by ensuring that blob content is properly closed using a defer statement. Feedback was provided to simplify the accompanying comment for better readability and to move historical context to the commit message.
Comment on lines
+179
to
+185
| // The ReadCloser returned by PullBlob was previously never closed on | ||
| // either path, leaking the underlying file descriptor (or HTTP body) | ||
| // for every blob we pushed. Close on the distribution implementation | ||
| // returns an error (#50), which is why we still wrap the reader with | ||
| // io.NopCloser below, but we DO need to close `content` ourselves. | ||
| // See #491. | ||
| defer content.Close() |
There was a problem hiding this comment.
The added comment is quite verbose and contains historical context that is better suited for a commit message. Additionally, it partially duplicates the explanation for the io.NopCloser workaround located just below. Consolidating this into a concise note about resource management would improve readability.
Suggested change
| // The ReadCloser returned by PullBlob was previously never closed on | |
| // either path, leaking the underlying file descriptor (or HTTP body) | |
| // for every blob we pushed. Close on the distribution implementation | |
| // returns an error (#50), which is why we still wrap the reader with | |
| // io.NopCloser below, but we DO need to close `content` ourselves. | |
| // See #491. | |
| defer content.Close() | |
| // Ensure the blob content is closed to avoid leaking resources (#491). | |
| // We use defer here and io.NopCloser below to work around the distribution | |
| // library's Close() implementation which returns a known error (#50). | |
| defer content.Close() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
pushIfNotExistpulls each blob's content from the source storage andthen hands it to
Blobs().Pushwrapped inio.NopCloser:The
io.NopCloseris intentional:Closeon the distribution readerreturns an error (see #50), so we don't let
Pushpropagate that error.But the wrapper also prevents the original
contentfrom ever beingclosed. On both success and error paths, the underlying file descriptor
(or HTTP body) leaks for every blob we push — and
Pushis called onceper object in the image. #491.
Fix
Add
defer content.Close()immediately after the nil-error check. Theexisting
NopCloserstill preventsPushfrom callingCloseon itsown, so the workaround for #50 is preserved.
deferhandles both thesuccess path and every early-return error path.
content, err := src.PullBlob(ctx, repo, desc.Digest.String()) if err != nil { return err } + defer content.Close() reader := pb.Add(prompt, desc.Digest.String(), desc.Size, content)Fixes #491