Deadlock when calling GetStatusCode() in SetOnLimitReached()

[jarv]

Calling GetStatusCode() which will acquire a RLock in the function called by ExecOnLimitReached() will cause a deadlock on concurrent requests.

The deadlock is fairly easy to reproduce using the code below and a request generator like ab.

My use-case for calling GetStatusCode() in the function passed to SetOnLimitReached() is to increment a prometheus counter that had a label value for the status code being used.

code to reproduce:

package main

import (
    "fmt"
    "log"
    "net/http"
    _ "net/http/pprof"
    "runtime"

    "github.com/didip/tollbooth/v7"
)

func testHandler(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "OK\n")
}


func main() {
    runtime.SetBlockProfileRate(1)
    runtime.SetMutexProfileFraction(5)
    listenStr := fmt.Sprintf(":%d", 6060)

    lmt := tollbooth.NewLimiter(float64(1), nil)
    log.Println("Setting rate limit for 1req/sec")
    lmt.SetIPLookups([]string{"RemoteAddr"})
    lmt.SetMessage("Your are sending requests too fast, slow down!")
    lmt.SetOnLimitReached(func(w http.ResponseWriter, r *http.Request) {
        log.Printf("Rate limit reached StatusCode: %d\n", lmt.GetStatusCode())
    })

    http.Handle("/", tollbooth.LimitFuncHandler(lmt, testHandler))

    log.Printf("Server started %s\n", listenStr)
    log.Fatal(http.ListenAndServe(listenStr, nil))
}

and the following to generate load to cause the deadlock:

ab -n 2000 -c 50 http://localhost:6060/'

Full backtrace (condensed using panicparse):

[jarv]

Note this is a recursive lock issue where two rlocks are acquired, if a lock occurs between the two rlocks then there will be a deadlock.

// ExecOnLimitReached is thread-safe way of executing after-rejection function when limit is reached.
func (l *Limiter) ExecOnLimitReached(w http.ResponseWriter, r *http.Request) {
    l.RLock()
    defer l.RUnlock()

    fn := l.onLimitReached
    if fn != nil {
        fn(w, r) // <--- any method calls that try to acquire a RLock() here will potentially cause a race
    }
}

https://pkg.go.dev/sync#RWMutex

It should not be used for recursive read locking; a blocked Lock call excludes new readers from acquiring the lock. See the documentation on the RWMutex type.

Probably the simplest fix for this would be to constrain the Rlock() so it is around the read on l.onLimitReached

 func (l *Limiter) ExecOnLimitReached(w http.ResponseWriter, r *http.Request) {
        l.RLock()
-       defer l.RUnlock()
-
        fn := l.onLimitReached
+       l.RUnlock()
        if fn != nil {
                fn(w, r)
        }