• 
      

    Attempt to handle threading-related issues with extension reloading.

    Review Request #5866 — Created May 22, 2014 and submitted

    Information

    Djblets
    release-0.8.x
    8891572...

    Reviewers

    We observed some issues in a mod_worker environment where attempts to
    reload extensions would fail, causing state to become corrupted, leading
    to more and more errors. Some of these cases were made more
    bullet-proof, but we didn't fix the core issue.

    This change attempts to prevent what may be causing the issue by
    allowing only one thread to reload the list of extensions at a time.

    We have a lock set up for reloading extensions (in case two users try to
    reload in the admin UI at the same time).

    There's a separate lock in the middleware that's a bit smarter about
    calling load(). It checks if the extension state has expired, tries to
    grab a lock, and then checks again for expiration. The goal there is to
    not have a bunch of threads that are waiting decide they're all going to
    trigger their own reloads.

    Unit tests have been written that hit issues both during uninit and
    during init of extensions. These mirror the errors we've been getting
    on reviews.reviewboard.org with Review Bot. With the fix, these tests
    pass.

    I don't have a real-world repro case yet (we'll see in a couple days on
    reviews.reviewboard.org if we start to hit any problems), but I was able
    to reproduce some of the errors we through unit tests.

    Basically, there's a number of possible failure points, depending on what
    instructions are being executed, but the tests manage to reliably trigger
    one error during init and one during uninit. Those trigger only if I turn off
    locking. Everything passes if locking is on.

    Description From Last Updated

    local variable 'extension' is assigned to but never used

    reviewbotreviewbot

    Col: 80 E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    local variable 'hook' is assigned to but never used

    reviewbotreviewbot

    local variable 'hook' is assigned to but never used

    reviewbotreviewbot

    Can we just get rid of the outer is_expired call and always do this with the lock acquired?

    daviddavid

    local variable 'hook' is assigned to but never used

    reviewbotreviewbot

    local variable 'hook' is assigned to but never used

    reviewbotreviewbot
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          djblets/extensions/manager.py
          djblets/extensions/middleware.py
          djblets/extensions/tests.py
        Ignored Files:
      
      
    2. djblets/extensions/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (80 > 79 characters)
      
    3. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          djblets/extensions/manager.py
          djblets/extensions/middleware.py
          djblets/extensions/tests.py
        Ignored Files:
      
      
    2. djblets/extensions/tests.py (Diff revision 1)
       
       
      Show all issues
       local variable 'extension' is assigned to but never used
      
    3. djblets/extensions/tests.py (Diff revision 1)
       
       
      Show all issues
       local variable 'hook' is assigned to but never used
      
    4. djblets/extensions/tests.py (Diff revision 1)
       
       
      Show all issues
       local variable 'hook' is assigned to but never used
      
    5. 
        
    chipx86
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          djblets/extensions/manager.py
          djblets/extensions/middleware.py
          djblets/extensions/tests.py
        Ignored Files:
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          djblets/extensions/manager.py
          djblets/extensions/middleware.py
          djblets/extensions/tests.py
        Ignored Files:
      
      
    2. djblets/extensions/tests.py (Diff revision 2)
       
       
      Show all issues
       local variable 'hook' is assigned to but never used
      
    3. djblets/extensions/tests.py (Diff revision 2)
       
       
      Show all issues
       local variable 'hook' is assigned to but never used
      
    4. 
        
    david
    1. 
        
    2. djblets/extensions/middleware.py (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      Can we just get rid of the outer is_expired call and always do this with the lock acquired?

      1. I didn't want to have threads wait around for no reason, even if it's for a very short period of time. Particularly if a thread is acting up for some reason.

      2. Maybe add a comment for that?

    3. 
        
    david
    1. Ship It!

    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-0.8.x (cb370c6)
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (cb370c6)