• 
      

    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)