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: Closed (submitted)

Change Summary:

Pushed to release-0.8.x (cb370c6)
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (cb370c6)
Loading...