-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent broken binary if download process is Interrupted #15
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @theamazinghari 👋
Thanks for your contribution 👍
This simple check could indeed prevent some invalid updates caused by an issue during the download.
I added a couple remarks to your implementation.
Let me know what do you think.
Cheers
// Use the same flags that ioutil.WriteFile uses | ||
f, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0755) | ||
f, err := os.OpenFile(tempFile.Name(), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0755) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you've created a variable let's reuse it
f, err := os.OpenFile(tempFile.Name(), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0755) | |
f, err := os.OpenFile(tempFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0755) |
if err != nil { | ||
os.Rename(destBackup, dest) | ||
_ = os.Remove(tempFile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ = os.Remove(tempFile.Name()) | |
_ = os.Remove(tempFilePath) |
|
||
fmt.Printf("s3update: updated with success to version %d\nRestarting application\n", remoteVersion) | ||
|
||
// The update completed, we can now restart the application without requiring any user action. | ||
if err := syscall.Exec(dest, os.Args, os.Environ()); err != nil { | ||
fmt.Println(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was temporary for debugging purpose, should be removed
fmt.Println(err) |
@@ -114,7 +116,7 @@ func runAutoUpdate(u Updater) error { | |||
if err != nil { | |||
return err | |||
} | |||
defer resp.Body.Close() | |||
remoteFileSize = *resp.ContentLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should still keep the call to resp.body.Close
to guarantee no matter each path we are taking this is properly closed
remoteFileSize = *resp.ContentLength | |
defer resp.Body.Close() | |
remoteFileSize = *resp.ContentLength |
} | ||
tempFilePath := tempFile.Name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also ensure the stream is properly closed no matter which path we take
tempFilePath := tempFile.Name() | |
defer tempFile.Close() | |
tempFilePath := tempFile.Name() |
return err | ||
} | ||
defer f.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I expect the defer to stays in there new implementation
@@ -17,6 +18,8 @@ import ( | |||
"github.com/mitchellh/ioprogress" | |||
) | |||
|
|||
var remoteFileSize int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace this global variable by variable local to the function, and pass it as argument to the function that requires it.
It can lead to issue on concurrent calls.
if downloadSucceeded(tempFilePath) { | ||
// Backup current binary | ||
if _, err = os.Stat(originalFilePath); err == nil { | ||
err = os.Rename(originalFilePath, backupFilePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What made you decide to perform a backup?
Could we simply stick to try to rename the temporary downloaded binary to the destination path?
What can fail (IO interruption, disk space, etc) in those call can still fail in subsequent calls so I'm not sure I understand the benefits.
func fileSize(path string) int64 { | ||
fileInfo, err := os.Stat(path) | ||
if err != nil { | ||
log.Fatal(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect to return an error and not exiting, this will enable package consumer to retry if they would like to.
@@ -89,12 +92,11 @@ func runAutoUpdate(u Updater) error { | |||
return fmt.Errorf("invalid local version") | |||
} | |||
|
|||
svc := s3.New(session.New(), &aws.Config{Region: aws.String(u.S3Region)}) | |||
svc := s3.New(session.Must(session.NewSession()), &aws.Config{Region: aws.String(u.S3Region)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for replacing the deprecated call 👍
Currently, if something happen during the download process, the binary file is corrupted and need to be manually restored.
I propose doing a very simple verification by checking the downloaded file size after downloading has finished, then proceed.