fix(utils): add error handling on file close and syncing data to storage #443
Conversation
VibhorGits
requested review from
heiruwu,
pinglin and
xiaofei-du
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #443 +/- ##
========================================
- Coverage 1.04% 1.03% -0.01%
========================================
Files 10 10
Lines 5373 5386 +13
========================================
Hits 56 56
- Misses 5309 5322 +13
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
VibhorGits
changed the title
heiruwu
changed the title
pkg/utils/file.go
Outdated
| @@ -277,10 +285,22 @@ func UpdateModelPath(modelDir string, dstDir string, owner string, model *datamo | |||
| return "", "", err | |||
| } | |||
| if _, err := io.Copy(dstFile, srcFile); err != nil { | |||
| srcFile.Close() // Close the source file in case of an error | |||
| dstFile.Close() // Close the destination file in case of an error | |||
There was a problem hiding this comment.
Hi @VibhorGits, these .Close() calls need to be handled as well.
I advise you to use defer function to avoid excessive error handling blocks, like this
defer func() {
// try to close the file; if an error occurs, log the error
if err := srcFile.Close(); err != nil {
log.Printf("%v", err.Error())
}
}()
|
Hey @heiruwu @tonywang10101 I made some changes in my pr adding defer function as per my understanding. |
| @@ -157,7 +157,15 @@ func Unzip(fPath string, dstDir string, owner string, uploadedModel *datamodel.M | |||
| return "", "", err | |||
| } | |||
|
|
|||
| dstFile.Close() | |||
| // Close the file handle and sync the data to the storage medium | |||
| if err := dstFile.Close(); err != nil { | |||
There was a problem hiding this comment.
Using defer instead of call it directly
| // Defer the closing and syncing of dstFile | ||
| defer func() { | ||
| if err := dstFile.Close(); err != nil { | ||
| log.Printf("%v", err.Error()) |
There was a problem hiding this comment.
log would be undefined here, suggestion:
defer func(logger *log.Logger) {
if err := dstFile.Close(); err != nil {
logger.Printf("%v", err.Error())
}
if err := dstFile.Sync(); err != nil {
logger.Printf("%v", err.Error())
}
}(log.Default()) // Pass the log object to the deferred function
|
Hey @tonywang10101 I made some changes and used logger as per your suggestion but there's something more work to be done considering the integration testing failing which I am not able to understand. 😕 |
Because
Handling Error while closing dst file and syncing the data to storage medium preventing data loss.