🐛 fix: Always close form file#3786
🐛 fix: Always close form file#3786ReneWerner87 merged 5 commits intogofiber:mainfrom arturmelanchyk:reader-close
Conversation
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
WalkthroughRefactors file upload handling in client/hooks.go by extracting logic into a new helper addFormFile, updating error messages and flow. Adds tests in client/hooks_test.go to validate error cases for file-based request bodies, including missing files and missing file names. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant P as parserRequestBodyFile
participant H as addFormFile
participant FS as FileSystem
participant MF as MultipartWriter
C->>P: parserRequestBodyFile(req)
loop For each file f
P->>H: addFormFile(form, f)
alt Reader is nil
H->>FS: Open(f.path)
FS-->>H: File handle or error
alt Open error
H-->>P: error "open file error"
P-->>C: error
else Open ok
note right of H: defer close reader
end
end
H->>MF: CreateFormFile(f.name, f.filename)
MF-->>H: formFile or error
alt CreateFormFile error
H-->>P: error
P-->>C: error
else
H->>MF: Copy(reader -> formFile)
alt Copy error
H-->>P: error
P-->>C: error
else
H-->>P: nil
end
end
end
P-->>C: nil
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3786 +/- ##
==========================================
- Coverage 91.67% 91.65% -0.03%
==========================================
Files 113 113
Lines 11959 11966 +7
==========================================
+ Hits 10964 10968 +4
- Misses 731 734 +3
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com> (cherry picked from commit 25df78f)
Signed-off-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a potential file descriptor leak by refactoring the file handling logic to use a defer statement, ensuring that file readers are always closed. The introduction of the addFormFile function improves code modularity. However, this change introduces a regression by ignoring the error returned by f.reader.Close(), which was previously handled. I've provided a suggestion to correctly handle this error while retaining the benefits of using defer.
| func addFormFile(mw *multipart.Writer, f *File, fileBuf *[]byte) error { | ||
| // If reader is not set, open the file. | ||
| if f.reader == nil { | ||
| var err error | ||
| f.reader, err = os.Open(f.path) | ||
| if err != nil { | ||
| return fmt.Errorf("create file error: %w", err) | ||
| return fmt.Errorf("open file error: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if _, err := io.CopyBuffer(w, v.reader, *fileBuf); err != nil { | ||
| return fmt.Errorf("failed to copy file data: %w", err) | ||
| } | ||
| // Ensure the file reader is always closed after copying. | ||
| defer f.reader.Close() //nolint:errcheck // not needed | ||
|
|
||
| if err := v.reader.Close(); err != nil { | ||
| return fmt.Errorf("close file error: %w", err) | ||
| } | ||
| // Create form file and copy the content. | ||
| w, err := mw.CreateFormFile(f.fieldName, f.name) | ||
| if err != nil { | ||
| return fmt.Errorf("create file error: %w", err) | ||
| } | ||
|
|
||
| if _, err := io.CopyBuffer(w, f.reader, *fileBuf); err != nil { | ||
| return fmt.Errorf("failed to copy file data: %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
While using defer is a great way to ensure the file is closed, ignoring the error from Close() is a regression from the previous behavior and is generally not recommended. The Close method can return important errors, for example, if there was an issue with the underlying file system. The nolint:errcheck comment states it's not needed, but the previous implementation did check this error.
I suggest modifying the function to handle the error from Close() while still benefiting from defer. This can be achieved using a named return value.
func addFormFile(mw *multipart.Writer, f *File, fileBuf *[]byte) (err error) {
// If reader is not set, open the file.
if f.reader == nil {
f.reader, err = os.Open(f.path)
if err != nil {
return fmt.Errorf("open file error: %w", err)
}
}
// Ensure the file reader is always closed after copying.
defer func() {
if closeErr := f.reader.Close(); closeErr != nil && err == nil {
err = fmt.Errorf("close file error: %w", closeErr)
}
}()
// Create form file and copy the content.
var w io.Writer
w, err = mw.CreateFormFile(f.fieldName, f.name)
if err != nil {
return fmt.Errorf("create file error: %w", err)
}
if _, err = io.CopyBuffer(w, f.reader, *fileBuf); err != nil {
return fmt.Errorf("failed to copy file data: %w", err)
}
return nil
}There was a problem hiding this comment.
Pull Request Overview
Fixes missing file Close calls when errors occur during multipart form file handling to ensure readers are always closed.
- Extracts file handling into addFormFile with a deferred Close to guarantee cleanup on all error paths.
- Adds tests for open failure and missing path/name validation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/hooks.go | Refactors file handling into addFormFile, adding deferred Close; adjusts loop variables and field defaults. |
| client/hooks_test.go | Adds tests for open error and missing path/name cases in file upload parsing. |
Co-authored-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
Description
Currently file doesn't get closed if any of errors at lines 299 or 303 return:
fiber/client/hooks.go
Lines 288 to 308 in e8f639a
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md