Pull request 1796: 5661-opt-more-lock

Updates #5661.

Squashed commit of the following:

commit 0a1425df0ea1278c73ac88cbee053897d3daaf1b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Mar 31 18:31:19 2023 +0300

    querylog: opt locks more

internal/querylog/http.go

@@ -82,15 +82,17 @@ func (l *queryLog) handleQueryLog(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	var resp map[string]any
+	var entries []*logEntry
+	var oldest time.Time
 	func() {
-		l.lock.Lock()
+		l.confMu.RLock()
-		defer l.lock.Unlock()
+		defer l.confMu.RUnlock()
 
-		entries, oldest := l.search(params)
+		entries, oldest = l.search(params)
-		resp = l.entriesToJSON(entries, oldest)
 	}()
 
+	resp := entriesToJSON(entries, oldest, l.anonymizer.Load())
+
 	_ = aghhttp.WriteJSONResponse(w, r, resp)
 }
 
@@ -105,8 +107,8 @@ func (l *queryLog) handleQueryLogClear(_ http.ResponseWriter, _ *http.Request) {
 //
 // Deprecated:  Remove it when migration to the new API is over.
 func (l *queryLog) handleQueryLogInfo(w http.ResponseWriter, r *http.Request) {
-	l.lock.Lock()
+	l.confMu.RLock()
-	defer l.lock.Unlock()
+	defer l.confMu.RUnlock()
 
 	ivl := l.conf.RotationIvl
 
@@ -128,8 +130,8 @@ func (l *queryLog) handleQueryLogInfo(w http.ResponseWriter, r *http.Request) {
 func (l *queryLog) handleGetQueryLogConfig(w http.ResponseWriter, r *http.Request) {
 	var resp *getConfigResp
 	func() {
-		l.lock.Lock()
+		l.confMu.RLock()
-		defer l.lock.Unlock()
+		defer l.confMu.RUnlock()
 
 		resp = &getConfigResp{
 			Interval:          float64(l.conf.RotationIvl.Milliseconds()),
@@ -137,10 +139,10 @@ func (l *queryLog) handleGetQueryLogConfig(w http.ResponseWriter, r *http.Reques
 			AnonymizeClientIP: aghalg.BoolToNullBool(l.conf.AnonymizeClientIP),
 			Ignored:           l.conf.Ignored.Values(),
 		}
-
-		slices.Sort(resp.Ignored)
 	}()
 
+	slices.Sort(resp.Ignored)
+
 	_ = aghhttp.WriteJSONResponse(w, r, resp)
 }
 
@@ -187,8 +189,8 @@ func (l *queryLog) handleQueryLogConfig(w http.ResponseWriter, r *http.Request)
 
 	defer l.conf.ConfigModified()
 
-	l.lock.Lock()
+	l.confMu.Lock()
-	defer l.lock.Unlock()
+	defer l.confMu.Unlock()
 
 	conf := *l.conf
 	if newConf.Enabled != aghalg.NBNull {
@@ -251,8 +253,8 @@ func (l *queryLog) handlePutQueryLogConfig(w http.ResponseWriter, r *http.Reques
 
 	defer l.conf.ConfigModified()
 
-	l.lock.Lock()
+	l.confMu.Lock()
-	defer l.lock.Unlock()
+	defer l.confMu.Unlock()
 
 	conf := *l.conf
 

internal/querylog/json.go

@@ -19,12 +19,16 @@ import (
 type jobject = map[string]any
 
 // entriesToJSON converts query log entries to JSON.
-func (l *queryLog) entriesToJSON(entries []*logEntry, oldest time.Time) (res jobject) {
+func entriesToJSON(
+	entries []*logEntry,
+	oldest time.Time,
+	anonFunc aghnet.IPMutFunc,
+) (res jobject) {
 	data := make([]jobject, 0, len(entries))
 
 	// The elements order is already reversed to be from newer to older.
 	for _, entry := range entries {
-		jsonEntry := l.entryToJSON(entry, l.anonymizer.Load())
+		jsonEntry := entryToJSON(entry, anonFunc)
 		data = append(data, jsonEntry)
 	}
 
@@ -40,7 +44,7 @@ func (l *queryLog) entriesToJSON(entries []*logEntry, oldest time.Time) (res job
 }
 
 // entryToJSON converts a log entry's data into an entry for the JSON API.
-func (l *queryLog) entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (jsonEntry jobject) {
+func entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (jsonEntry jobject) {
 	hostname := entry.QHost
 	question := jobject{
 		"type":  entry.QType,
@@ -92,14 +96,14 @@ func (l *queryLog) entryToJSON(entry *logEntry, anonFunc aghnet.IPMutFunc) (json
 		jsonEntry["service_name"] = entry.Result.ServiceName
 	}
 
-	l.setMsgData(entry, jsonEntry)
+	setMsgData(entry, jsonEntry)
-	l.setOrigAns(entry, jsonEntry)
+	setOrigAns(entry, jsonEntry)
 
 	return jsonEntry
 }
 
 // setMsgData sets the message data in jsonEntry.
-func (l *queryLog) setMsgData(entry *logEntry, jsonEntry jobject) {
+func setMsgData(entry *logEntry, jsonEntry jobject) {
 	if len(entry.Answer) == 0 {
 		return
 	}
@@ -122,7 +126,7 @@ func (l *queryLog) setMsgData(entry *logEntry, jsonEntry jobject) {
 }
 
 // setOrigAns sets the original answer data in jsonEntry.
-func (l *queryLog) setOrigAns(entry *logEntry, jsonEntry jobject) {
+func setOrigAns(entry *logEntry, jsonEntry jobject) {
 	if len(entry.OrigAnswer) == 0 {
 		return
 	}

internal/querylog/qlog.go

@@ -25,9 +25,12 @@ const (
 type queryLog struct {
 	findClient func(ids []string) (c *Client, err error)
 
-	conf    *Config
+	// confMu protects conf.
-	lock    sync.Mutex
+	confMu *sync.RWMutex
-	logFile string // path to the log file
+	conf   *Config
+
+	// logFile is the path to the log file.
+	logFile string
 
 	// bufferLock protects buffer.
 	bufferLock sync.RWMutex
@@ -279,8 +282,8 @@ func (l *queryLog) Add(params *AddParams) {
 
 // ShouldLog returns true if request for the host should be logged.
 func (l *queryLog) ShouldLog(host string, _, _ uint16) bool {
-	l.lock.Lock()
+	l.confMu.RLock()
-	defer l.lock.Unlock()
+	defer l.confMu.RUnlock()
 
 	return !l.isIgnored(host)
 }

internal/querylog/qlog_test.go

@@ -259,11 +259,11 @@ func TestQueryLogShouldLog(t *testing.T) {
 	set := stringutil.NewSet(ignored1, ignored2)
 
 	l, err := newQueryLog(Config{
+		Ignored:     set,
 		Enabled:     true,
 		RotationIvl: timeutil.Day,
 		MemSize:     100,
 		BaseDir:     t.TempDir(),
-		Ignored:     set,
 	})
 	require.NoError(t, err)
 

internal/querylog/querylog.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net"
 	"path/filepath"
+	"sync"
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
@@ -33,11 +34,14 @@ type QueryLog interface {
 
 // Config is the query log configuration structure.
 type Config struct {
+	// Ignored is the list of host names, which should not be written to log.
+	Ignored *stringutil.Set
+
 	// Anonymizer processes the IP addresses to anonymize those if needed.
 	Anonymizer *aghnet.IPMut
 
-	// ConfigModified is called when the configuration is changed, for
+	// ConfigModified is called when the configuration is changed, for example
-	// example by HTTP requests.
+	// by HTTP requests.
 	ConfigModified func()
 
 	// HTTPRegister registers an HTTP handler.
@@ -49,20 +53,13 @@ type Config struct {
 	// BaseDir is the base directory for log files.
 	BaseDir string
 
-	// RotationIvl is the interval for log rotation.  After that period, the
+	// RotationIvl is the interval for log rotation.  After that period, the old
-	// old log file will be renamed, NOT deleted, so the actual log
+	// log file will be renamed, NOT deleted, so the actual log retention time
-	// retention time is twice the interval.  The value must be one of:
+	// is twice the interval.
-	//
-	//    6 * time.Hour
-	//    1 * timeutil.Day
-	//    7 * timeutil.Day
-	//   30 * timeutil.Day
-	//   90 * timeutil.Day
-	//
 	RotationIvl time.Duration
 
-	// MemSize is the number of entries kept in a memory buffer before they
+	// MemSize is the number of entries kept in a memory buffer before they are
-	// are flushed to disk.
+	// flushed to disk.
 	MemSize uint32
 
 	// Enabled tells if the query log is enabled.
@@ -74,10 +71,6 @@ type Config struct {
 	// AnonymizeClientIP tells if the query log should anonymize clients' IP
 	// addresses.
 	AnonymizeClientIP bool
-
-	// Ignored is the list of host names, which should not be written to
-	// log.
-	Ignored *stringutil.Set
 }
 
 // AddParams is the parameters for adding an entry.
@@ -150,11 +143,13 @@ func newQueryLog(conf Config) (l *queryLog, err error) {
 	l = &queryLog{
 		findClient: findClient,
 
-		logFile:    filepath.Join(conf.BaseDir, queryLogFileName),
+		conf:    &Config{},
+		confMu:  &sync.RWMutex{},
+		logFile: filepath.Join(conf.BaseDir, queryLogFileName),
+
 		anonymizer: conf.Anonymizer,
 	}
 
-	l.conf = &Config{}
 	*l.conf = conf
 
 	err = validateIvl(conf.RotationIvl)

internal/querylog/querylogfile.go

@@ -167,8 +167,8 @@ func (l *queryLog) periodicRotate() {
 // checkAndRotate rotates log files if those are older than the specified
 // rotation interval.
 func (l *queryLog) checkAndRotate() {
-	l.lock.Lock()
+	l.confMu.RLock()
-	defer l.lock.Unlock()
+	defer l.confMu.RUnlock()
 
 	oldest, err := l.readFileFirstTimeValue()
 	if err != nil && !errors.Is(err, os.ErrNotExist) {