Skip to content

Commit

Permalink
Prevent the router from deadlocking itself when calling Commit()
Browse files Browse the repository at this point in the history
The router reload function (Commit()) was changed so that it could be
rate limited.  The rate limiter tracks changes in a kcache.Queue
object.  It will coalesce changes to the same call so that if three
calls to Invoke() the rate limited function happen before the next
time it is allowed to process, then only one will occur.

Our problem was that we were doing:
 Thread 1 (the rate-limiter background process):
   - Wake up
   - Ratelimit sees there is work to be done and calls fifo.go's Pop() function
   - Fifo.go acquires a fifo lock and call the processing function
   - Router.go's commitAndReload() function acquires a lock on the router object
 Thread 2 (triggered by the event handler that commit's changes to the router):
   - Get the event and process it
   - Since there are changes to be made, call router.Commit()
   - Commit() grabs a lock on the router object
   - Then calls the rate-limiter wrapper around commitAndReload() using Invoke() to queue work
   - In order to queue the work... it acquires a lock on the fifo

So thread 1 locks: fifo then router; thread 2 locks: router then fifo.
If you get unlucky, those threads deadlock and you never process
another event.

The fix is to release the lock on the router object in our Commit()
function before we call Invoke on the rate limited function.  The lock
is not actually protecting anything at that point since the rate
limited function does its own locking, and is run in a separate thread
anyway.

Fixes bug 1440977 (https://bugzilla.redhat.com/show_bug.cgi?id=1440977)
  • Loading branch information
knobunc committed Apr 12, 2017
1 parent b3cacc5 commit 7dc887d
Showing 1 changed file with 7 additions and 3 deletions.
10 changes: 7 additions & 3 deletions pkg/router/template/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,18 +386,20 @@ func (r *templateRouter) readState() error {
// the state and refresh the backend. This is all done in the background
// so that we can rate limit + coalesce multiple changes.
func (r *templateRouter) Commit() {

r.lock.Lock()
defer r.lock.Unlock()

if !r.synced {
glog.V(4).Infof("Router state synchronized for the first time")
r.synced = true
r.stateChanged = true
}

if r.stateChanged {
needsCommit := r.stateChanged
r.lock.Unlock()

if needsCommit {
r.rateLimitedCommitFunction.Invoke(r.rateLimitedCommitFunction)
r.stateChanged = false
}
}

Expand All @@ -413,6 +415,8 @@ func (r *templateRouter) commitAndReload() error {
return err
}

r.stateChanged = false

glog.V(4).Infof("Writing the router config")
reloadStart := time.Now()
err := r.writeConfig()
Expand Down

0 comments on commit 7dc887d

Please sign in to comment.