Replace Mutex with RWMutex

Dockerfile

@@ -13,7 +13,7 @@ RUN set -x \
 
 RUN mkdir -p /opt/banjax
 COPY ./ /opt/banjax/
-RUN cd /opt/banjax && go test && go build
+RUN cd /opt/banjax && go test && go build -o banjax -tags debug
 
 RUN mkdir -p /etc/banjax
 COPY ./banjax-config.yaml /etc/banjax/
@@ -31,7 +31,7 @@ WORKDIR /opt/banjax
 
 # To enable live reload for dev, uncomment the following lines
 # COPY ./.air.toml /opt/banjax/
-# RUN go install github.com/air-verse/air@latest
+# RUN go install github.com/air-verse/air@v1.52.3
 # RUN mkdir -p /opt/banjax/tmp
 # CMD ["air", "-c", ".air.toml"]
 CMD ["./banjax"]

banjax.go

@@ -171,14 +171,14 @@ func init_ipset(config *internal.Config) ipset.IPSet {
 func main() {
 	// XXX protects ipToRegexStates and failedChallengeStates
 	// (why both? because there are too many parameters already?)
-	var rateLimitMutex sync.Mutex
+	var rateLimitMutex sync.RWMutex
 	ipToRegexStates := internal.IpToRegexStates{}
 	failedChallengeStates := internal.FailedChallengeStates{}
 
 	var passwordProtectedPaths internal.PasswordProtectedPaths
 
 	// XXX protects decisionLists
-	var decisionListsMutex sync.Mutex
+	var decisionListsMutex sync.RWMutex
 	var decisionLists internal.DecisionLists
 
 	standaloneTestingPtr := flag.Bool("standalone-testing", false, "makes it easy to test standalone")

docker-compose.yml

@@ -33,3 +33,13 @@ services:
       dockerfile: Dockerfile
     ports:
       - "8080:8080"
+
+  # wrk:
+  #   image: ghcr.io/william-yeh/wrk
+  #   platform: linux/amd64
+  #   volumes:
+  #     - ./tmp:/data
+  #   command: -t12 -c1024 -d10s http://localhost
+  #   # Automatically removes the container when it exits
+  #   restart: "no"
+  #   network_mode: "service:nginx"

go.mod

@@ -25,6 +25,7 @@ require (
 
 require (
 	github.com/brianvoe/gofakeit/v6 v6.16.0
+	github.com/gin-contrib/pprof v1.5.0
 	github.com/gonetx/ipset v0.1.0
 	github.com/jeremy5189/ipfilter-no-iploc/v2 v2.0.3
 	github.com/stretchr/testify v1.9.0

go.sum

@@ -18,6 +18,8 @@ github.com/fsnotify/fsnotify v1.5.1 h1:mZcQUHVQUQWoPXXtuf9yuEXKudkV2sx1E06UadKWp
 github.com/fsnotify/fsnotify v1.5.1/go.mod h1:T3375wBYaZdLLcVNkcVbzGHY7f1l/uK5T5Ai1i3InKU=
 github.com/gabriel-vasile/mimetype v1.4.5 h1:J7wGKdGu33ocBOhGy0z653k/lFKLFDPJMG8Gql0kxn4=
 github.com/gabriel-vasile/mimetype v1.4.5/go.mod h1:ibHel+/kbxn9x2407k1izTA1S81ku1z/DlgOW2QE0M4=
+github.com/gin-contrib/pprof v1.5.0 h1:E/Oy7g+kNw94KfdCy3bZxQFtyDnAX2V7axRS7sNYVrU=
+github.com/gin-contrib/pprof v1.5.0/go.mod h1:GqFL6LerKoCQ/RSWnkYczkTJ+tOAUVN/8sbnEtaqOKs=
 github.com/gin-contrib/sse v0.1.0 h1:Y/yl/+YNO8GZSjAhjMsSuLt29uWRFHdHYUb5lYOV9qE=
 github.com/gin-contrib/sse v0.1.0/go.mod h1:RHrZQHXnP2xjPF+u1gW/2HnVO7nvIa9PG3Gm+fLHvGI=
 github.com/gin-gonic/gin v1.10.0 h1:nTuyha1TYqgedzytsKYqna+DfLos46nTv2ygFy86HFU=

internal/config.go

@@ -451,7 +451,7 @@ func checkExpiringDecisionListsByDomain(domain string, decisionLists *DecisionLi
 
 // XXX mmm could hold the lock for a while?
 func RemoveExpiredDecisions(
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) {
 	decisionListsMutex.Lock()
@@ -466,7 +466,7 @@ func RemoveExpiredDecisions(
 }
 
 func removeExpiredDecisionsByIp(
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	ip string,
 ) {
@@ -480,7 +480,7 @@ func removeExpiredDecisionsByIp(
 func updateExpiringDecisionLists(
 	config *Config,
 	ip string,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	expires time.Time,
 	newDecision Decision,
@@ -514,7 +514,7 @@ func updateExpiringDecisionListsSessionId(
 	config *Config,
 	ip string,
 	sessionId string,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	expires time.Time,
 	newDecision Decision,
@@ -550,14 +550,14 @@ type MetricsLogLine struct {
 
 func WriteMetricsToEncoder(
 	metricsLogEncoder *json.Encoder,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	ipToRegexStates *IpToRegexStates,
 	failedChallengeStates *FailedChallengeStates,
 ) {
-	decisionListsMutex.Lock()
-	defer decisionListsMutex.Unlock()
+	decisionListsMutex.RLock()
+	defer decisionListsMutex.RUnlock()
 
 	lenExpiringChallenges := 0
 	lenExpiringBlocks := 0

internal/http_server.go

@@ -18,6 +18,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/gin-contrib/pprof"
 	"github.com/gin-gonic/gin"
 )
 
@@ -28,10 +29,10 @@ const (
 
 func RunHttpServer(
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	passwordProtectedPaths *PasswordProtectedPaths,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	ipToRegexStates *IpToRegexStates,
 	failedChallengeStates *FailedChallengeStates,
 	banner BannerInterface,
@@ -54,6 +55,9 @@ func RunHttpServer(
 	}
 
 	r := gin.New()
+	pprof.Register(r)
+	runtime.SetBlockProfileRate(1)
+	runtime.SetMutexProfileFraction(1)
 
 	type LogLine struct {
 		Time          string
@@ -189,14 +193,14 @@ func RunHttpServer(
 	})
 
 	r.GET("/rate_limit_states", func(c *gin.Context) {
-		rateLimitMutex.Lock()
+		rateLimitMutex.RLock()
 		c.String(200,
 			fmt.Sprintf("regexes:\n%v\nfailed challenges:\n%v",
 				ipToRegexStates.String(),
 				failedChallengeStates.String(),
 			),
 		)
-		rateLimitMutex.Unlock()
+		rateLimitMutex.RUnlock()
 	})
 
 	// API to check if given IP was banned by iptables
@@ -484,10 +488,10 @@ func tooManyFailedChallenges(
 	path string,
 	banner BannerInterface,
 	challengeType string,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	failedChallengeStates *FailedChallengeStates,
 	method string,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) (tooManyFailedChallengesResult TooManyFailedChallengesResult) {
 	rateLimitMutex.Lock()
@@ -588,10 +592,10 @@ func sendOrValidateShaChallenge(
 	config *Config,
 	c *gin.Context,
 	banner BannerInterface,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	failedChallengeStates *FailedChallengeStates,
 	failAction FailAction,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) (sendOrValidateShaChallengeResult SendOrValidateShaChallengeResult) {
 	clientIp := c.Request.Header.Get("X-Client-IP")
@@ -691,9 +695,9 @@ func sendOrValidatePassword(
 	passwordProtectedPaths *PasswordProtectedPaths,
 	c *gin.Context,
 	banner BannerInterface,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	failedChallengeStates *FailedChallengeStates,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) (sendOrValidatePasswordResult SendOrValidatePasswordResult) {
 	clientIp := c.Request.Header.Get("X-Client-IP")
@@ -833,10 +837,10 @@ type DecisionForNginxResult struct {
 
 func decisionForNginx(
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	passwordProtectedPaths *PasswordProtectedPaths,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	failedChallengeStates *FailedChallengeStates,
 	banner BannerInterface,
 ) gin.HandlerFunc {
@@ -868,16 +872,16 @@ func decisionForNginx(
 
 func checkPerSiteDecisionLists(
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	requestedHost string,
 	clientIp string,
 ) (bool, Decision) {
 	// XXX ugh this locking is awful
 	// i got bit by just checking against the zero value here, which is a valid iota enum
-	decisionListsMutex.Lock()
+	decisionListsMutex.RLock()
 	decision, ok := (*decisionLists).PerSiteDecisionLists[requestedHost][clientIp]
-	decisionListsMutex.Unlock()
+	decisionListsMutex.RUnlock()
 
 	// found as plain IP form, no need to check IPFilter
 	if ok {
@@ -907,10 +911,10 @@ func checkPerSiteDecisionLists(
 func decisionForNginx2(
 	c *gin.Context,
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	passwordProtectedPaths *PasswordProtectedPaths,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	failedChallengeStates *FailedChallengeStates,
 	banner BannerInterface,
 ) (decisionForNginxResult DecisionForNginxResult) {
@@ -1020,9 +1024,9 @@ func decisionForNginx2(
 		}
 	}
 
-	decisionListsMutex.Lock()
+	decisionListsMutex.RLock()
 	decision, ok = (*decisionLists).GlobalDecisionLists[clientIp]
-	decisionListsMutex.Unlock()
+	decisionListsMutex.RUnlock()
 	foundInIpFilter := false
 	if !ok {
 		for _, iterateDecision := range []Decision{Allow, Challenge, NginxBlock, IptablesBlock} {
@@ -1074,9 +1078,9 @@ func decisionForNginx2(
 	// when we insert something into the list, really we might just be extending the expiry time and/or
 	// changing the decision.
 	// XXX i forget if that comment is stale^
-	decisionListsMutex.Lock()
+	decisionListsMutex.RLock()
 	expiringDecision, ok := checkExpiringDecisionLists(c, clientIp, decisionLists)
-	decisionListsMutex.Unlock()
+	decisionListsMutex.RUnlock()
 	if !ok {
 		// log.Println("no mention in expiring lists")
 	} else {
@@ -1118,9 +1122,9 @@ func decisionForNginx2(
 
 	// the legacy banjax_sha_inv and user_banjax_sha_inv
 	// difference is one blocks after many failures and the other doesn't
-	decisionListsMutex.Lock()
+	decisionListsMutex.RLock()
 	failAction, ok := (*decisionLists).SitewideShaInvList[requestedHost]
-	decisionListsMutex.Unlock()
+	decisionListsMutex.RUnlock()
 	if !ok {
 		// log.Println("no mention in sitewide list")
 	} else {

internal/iptables.go

@@ -126,7 +126,7 @@ type BannerInterface interface {
 }
 
 type Banner struct {
-	DecisionListsMutex *sync.Mutex
+	DecisionListsMutex *sync.RWMutex
 	DecisionLists      *DecisionLists
 	Logger             *log.Logger
 	LoggerTemp         *log.Logger

internal/kafka.go

@@ -81,7 +81,7 @@ func getDialer(config *Config) *kafka.Dialer {
 
 func RunKafkaReader(
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	wg *sync.WaitGroup,
 ) {
@@ -158,7 +158,7 @@ func getBlockSessionTtl(config *Config, host string) (blockSessionTtl int) {
 func handleCommand(
 	config *Config,
 	command commandMessage,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) {
 	// exempt a site from baskerville according to config
@@ -191,7 +191,7 @@ func handleCommand(
 func handleIPCommand(
 	config *Config,
 	command commandMessage,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	decision Decision,
 	expireDuration int,
@@ -219,7 +219,7 @@ func handleIPCommand(
 func handleSessionCommand(
 	config *Config,
 	command commandMessage,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	decision Decision,
 	expireDuration int,

internal/regex_rate_limiter.go

@@ -22,9 +22,9 @@ import (
 func RunLogTailer(
 	config *Config,
 	banner BannerInterface,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	ipToRegexStates *IpToRegexStates,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 	wg *sync.WaitGroup,
 ) {
@@ -120,12 +120,12 @@ func parseTimestamp(timeIpRest []string) (timestamp time.Time, err error) {
 
 func checkIpInGlobalDecisionList(
 	ipString string,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 )(bool) {
 	// Check if IP is in the global allow list that should be skipped
-	decisionListsMutex.Lock()
-	defer decisionListsMutex.Unlock()
+	decisionListsMutex.RLock()
+	defer decisionListsMutex.RUnlock()
 
 	decision, ok := (*decisionLists).GlobalDecisionLists[ipString]
 	if (ok && decision == Allow) {
@@ -146,11 +146,11 @@ func checkIpInGlobalDecisionList(
 func checkIpInPerSiteDecisionList(
 	urlString string,
 	ipString string,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) (bool) {
-	decisionListsMutex.Lock()
-	defer decisionListsMutex.Unlock()
+	decisionListsMutex.RLock()
+	defer decisionListsMutex.RUnlock()
 
 	decision, ok := (*decisionLists).PerSiteDecisionLists[urlString][ipString]
 	if (ok && decision == Allow) {
@@ -177,11 +177,11 @@ func checkIpInPerSiteDecisionList(
 // parsing these unescaped space-separated strings is gross. maybe pass json instead.
 func consumeLine(
 	line *tail.Line,
-	rateLimitMutex *sync.Mutex,
+	rateLimitMutex *sync.RWMutex,
 	ipToRegexStates *IpToRegexStates,
 	banner BannerInterface,
 	config *Config,
-	decisionListsMutex *sync.Mutex,
+	decisionListsMutex *sync.RWMutex,
 	decisionLists *DecisionLists,
 ) (consumeLineResult ConsumeLineResult) {