Skip to content
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

fix: added daemon functions to the auto-updater #1509

Merged
merged 3 commits into from
May 21, 2020

Conversation

rafaelramalho19
Copy link
Contributor

Fixes #1508, fixes #1496, fixes #1375

  • I'm not able to test this for sure because autoUpdater doesn't work on dev mode.

The problem was that the autoUpdater was trying to access the stopIpfs from the context (ctx variable), but that would only be initialized with the functions after setupDaemon has been executed.

@rafaelramalho19 rafaelramalho19 self-assigned this May 18, 2020
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (makes sense mentally); but leaving this as a ref #1385. Update: Also, I think checkForUpdates is used somewhere in Tray so if the user is fast enough to call the function, it may break. I'd check that.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this should fix stopIpfs call in:

https://github.com/ipfs-shipyard/ipfs-desktop/blob/6175b7bc0202f85f2d27ab105124b495f59bd824/src/auto-updater/quit-and-install.js#L13

However, note that auto update is the critical feature of Desktop app and must be extremely robust. Right now, afaik, if stopIpfs throws, update does not happen.

To be on the safe side I'd tweak quit-and-install.js and wrap that stopIpfs call in try/catch/finally and put autoUpdater.quitAndInstall in the finally block, so it is ALWAYS executed – even if unexpected error occurs during stopIpfs, like we observe now, or in the future, when we refactor things around.

Thoughts?

src/auto-updater/quit-and-install.js Outdated Show resolved Hide resolved
@rafaelramalho19 rafaelramalho19 mentioned this pull request May 20, 2020
@rafaelramalho19 rafaelramalho19 linked an issue May 20, 2020 that may be closed by this pull request
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @rafaelramalho19 feel free to merge & release
(I believe this is important enough to be shipped as patch release v0.11.4)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants