Fix various bugs on program shutdown and races

KonishchevDmitry commented 7 years ago

This PR fixes the following issues:

A race on shutdown when repo is closing in goroutine, but the main goroutine (and the whole program) shutdowns before the closing goroutine finishes.
A memory leak: a new sigchan has been created on each run() and added to signal.Notify() without later removing.
Remove partially downloaded files on program interruption.
Fix a race due to which an invalid path may be written to media list file because we used the first guessed file name, but not the final one obtained from reserveUniqueFilename().
Fix a bug due to which an invalid path may be written to media list in case the file which it will points to fails to download.
Handle SIGTERM properly (for example in case of running under systemd).

mholt commented 7 years ago

Wow, thanks for the PR, @KonishchevDmitry! I will look at this later today or this weekend (I have a pressing paper deadline).

mholt commented 7 years ago

I have started reviewing this, but if you want faster reviews, could you split this into multiple, smaller PRs? Those are much easier to review and will get through my queue sooner. :)

KonishchevDmitry commented 7 years ago

Thank you for looking on my changes, @mholt! I don't mind to wait some time if you don't have a time now to review this. Sorry, I understand that smaller PR is better PR, but actually I don't know how to split this one into a smaller ones because actually this PR is all about the single feature - delete temporary files on clean shutdown. I've started to implement it and noticed that I need to repair current clean shutdown implementation + introduce some special version of Repository.Close() (Repository.CloseUnsafeOnExit()) and more sophisticated Repository.downloading: it needs a mutex which should be locked every time the downloading file is creating, deleting or changing its destination and a channel to notify about completion of downloading process. As I've changed Repository.downloading, all subsequent changes become inseparable from each other.

The PR contains 3 commits - that's how I tried to reduce the size of changes you'll have to review, but I decided to not create a separate PR for each commit because I'd had to create the new PR only after you approve the previous one (since it depends on previous commit changes), but if you don't see the last commit, you likely will be confused why we need all this stuff that I introduced in the first commit.

So could you please try to look at the changes commit-by-commit?

KonishchevDmitry commented 7 years ago

I've managed to extract one commit from the latest one as https://github.com/mholt/photobak/pull/5. All other changes seem to be inseparable for me.

KonishchevDmitry commented 7 years ago

Thank you for your review! Yes, sure, I would be glad to help.

mholt / photobak

Fix various bugs on program shutdown and races #3