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 “Update all apps” #451

Merged
merged 3 commits into from
Aug 3, 2020
Merged

Conversation

datenreisender
Copy link
Contributor

Fixes the bug NCP-3115.

When users press “Update all apps” sometimes an error message “Unable to load apps” was shown (even though it worked correctly).

This was caused by a race condition: When one app was completely upgraded it reloaded the state of all apps but another one was possible in the middle of being upgraded, which could lead to it's app folder being transiently in an inconsistent state. E.g. while deleting the old version, the package.json was already gone but the folder still existed.

To fix this, when removing the old version of the app, the app folder is first moved to different, temporary folder and the contents are removed there. When installing the new version it is done the other way around: The downloaded app is first unpacked to a temporary folder and afterwards the whole folder is moved to the final destination.

This PR bumps the version up to 3.4.2, but from my point of view this small fix alone does not yet justify doing a release. We can wait for the next thing that should also be released.

This change also makes it possible, that we could allow users to start multiple installs, upgrades or removals in parallel, but I will do that change in a different PR. Currently the UI state can be slightly broken in that regard anyhow: After users click on “Update all apps” all install/upgrade/remove buttons are temporarily disabled but as soon as the first upgrade is finished all buttons (including “Update all apps”) are enabled again.

When upgrading all apps we got errors because while one app was finished
upgrading another was still in progress, which exposed a temporary state
with an app folder being partially cleared or partially installed. So
these two operations are now atomic:
- When removing an app, that app folder is first moved to a temporary
  place, so that this happens instantly, and then the folder is cleared
  there.
- When installing an app, the app folder is first created and filled in
  a temporary place and then at the end moved to the final location, so
  that this also happens instantly.
Copy link
Contributor

@bencefr bencefr left a comment

Choose a reason for hiding this comment

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

LGTM

@datenreisender datenreisender merged commit abaa1e4 into release/3.4 Aug 3, 2020
@datenreisender datenreisender deleted the fix_update_all_apps branch August 3, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants