Improve extension static media installation and recover from failures.

Review Request #10964 — Created March 23, 2020 and submitted

Information

Djblets
release-1.0.x

Reviewers

When attempting to install static media files for an extension, some
things could go wrong, and we didn't always recover gracefully. There
were a few major issues:

  • If there were permission issues with static media files, but we could
    write the .version file, then only the .version would be written,
    preventing future attempts at installation.
  • If a lock was not released, installation would stall forever.
  • When installation failed, the administrator would receive a pretty
    lousy and useless error message at best, and see a request never
    complete (due to a HTTP 500) at worst.
  • If we had a permission error getting a lock file, things would blow up
    in unhelpful ways.
  • If static media files were missing, pages could completely crash
    trying to locate them.

This change fully rewrites the media installation code to deal with
these issues. It now considers both a locking failure and a
permission/IO error writing a lock file to be the same problem, and will
retry. The retry count is now capped at 10 tries, so we don't spin
forever. (This will still mean up to 10 seconds of stalling, as a
worst-case scenario.)

The .version file is only written once we have successfully installed
static media, so we don't appear to succeed when we fail.

If any part of this process goes wrong, a useful error message (often
containing instructions) is sent to the user (if enabling via the
administration UI) and logged. These will also cause the extension to
disable, so it's (hopefully) not in a half-installed state.

We also now check the final result to see if we've successfully bumped
the version, and provide a suitable error if we weren't able to *due to
permission/lock issues, for instance).

When loading an extension's static bundle that we can't find (which
should be less of an issue now), we no longer dump a stack trace and
raise the exception (breaking the page). Instead, we log a more useful
"error"-level warning and just give up trying to load that static media
files. This means that extension functionality on pages may not function
correctly, but the pages will at least load without an HTTP 500.

Unit tests passed.

Manually tested with a production site. Verified that initial installation
worked, that upgrades worked, and that the right errors were provided when
permissions were bad.

Summary ID
Improve extension static media installation and recover from failures.
When attempting to install static media files for an extension, some things could go wrong, and we didn't always recover gracefully. There were a few major issues: * If there were permission issues with static media files, but we could write the `.version` file, then only the `.version` would be written, preventing future attempts at installation. * If a lock was not released, installation would stall forever. * When installation failed, the administrator would receive a pretty lousy and useless error message at best, and see a request never complete (due to a HTTP 500) at worst. * If we had a permission error getting a lock file, things would blow up in unhelpful ways. * If static media files were missing, pages could completely crash trying to locate them. This change fully rewrites the media installation code to deal with these issues. It now considers both a locking failure _and_ a permission/IO error writing a lock file to be the same problem, and will retry. The retry count is now capped at 10 tries, so we don't spin forever. (This will still mean up to 10 seconds of stalling, as a worst-case scenario.) The `.version` file is only written once we have successfully installed static media, so we don't appear to succeed when we fail. If any part of this process goes wrong, a useful error message (often containing instructions) is sent to the user (if enabling via the administration UI) and logged. These will also cause the extension to disable, so it's (hopefully) not in a half-installed state. We also now check the final result to see if we've successfully bumped the version, and provide a suitable error if we weren't able to *due to permission/lock issues, for instance). When loading an extension's static bundle that we can't find (which should be less of an issue now), we no longer dump a stack trace and raise the exception (breaking the page). Instead, we log a more useful "error"-level warning and just give up trying to load that static media files. This means that extension functionality on pages may not function correctly, but the pages will at least load without an HTTP 500.
6f431d251bf6c022791d593fa5916f00b7d55619
Description From Last Updated

F841 local variable 'e' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'InstallExtensionMediaError'

reviewbotreviewbot

F821 undefined name 'lockfile'

reviewbotreviewbot

F821 undefined name 'ext_class'

reviewbotreviewbot

F401 'pkg_resources' imported but unused

reviewbotreviewbot

F401 'djblets.extensions.errors.InstallExtensionMediaError' imported but unused

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (97a1501)