Prevent broken binary if download process is Interrupted #15
Conversation
luphaz
left a comment
luphaz
left a comment
There was a problem hiding this comment.
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
| // Download to tempFile | ||
| // 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.
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) |
| f, err := os.OpenFile(tempFile.Name(), 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.
| _ = os.Remove(tempFile.Name()) | |
| _ = os.Remove(tempFilePath) |
|
|
||
| // 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.
I believe this was temporary for debugging purpose, should be removed
| fmt.Println(err) |
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
| remoteFileSize = *resp.ContentLength |
There was a problem hiding this comment.
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 |
| if err != nil { | ||
| return err | ||
| } | ||
| tempFilePath := tempFile.Name() |
There was a problem hiding this comment.
Let's also ensure the stream is properly closed no matter which path we take
| tempFilePath := tempFile.Name() | |
| defer tempFile.Close() | |
| tempFilePath := tempFile.Name() |
| if err := tempFile.Close(); err != nil { | ||
| return err | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
Same here, I expect the defer to stays in there new implementation
| "github.com/mitchellh/ioprogress" | ||
| ) | ||
|
|
||
| var remoteFileSize int64 |
There was a problem hiding this comment.
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.
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.
I would expect to return an error and not exiting, this will enable package consumer to retry if they would like to.
| } | ||
|
|
||
| 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.
Thanks for replacing the deprecated call 👍
2 participants
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.