filtering: imp code

internal/filtering/filter.go

@@ -74,7 +74,7 @@ func (d *DNSFilter) filterSetProperties(
 	listURL string,
 	newList FilterYAML,
 	isAllowlist bool,
-) (restart bool, err error) {
+) (shouldRestart bool, err error) {
 	d.filtersMu.Lock()
 	defer d.filtersMu.Unlock()
 
@@ -90,11 +90,11 @@ func (d *DNSFilter) filterSetProperties(
 
 	filt := &filters[i]
 	log.Debug(
-		"filter: set properties: %s: {%s %s %v}",
-		filt.URL,
+		"set name to %q, url to %s, enabled to %t for filter %s",
 		newList.Name,
 		newList.URL,
 		newList.Enabled,
+		filt.URL,
 	)
 
 	defer func(oldURL, oldName string, oldEnabled bool, oldUpdated time.Time) {
@@ -109,11 +109,11 @@ func (d *DNSFilter) filterSetProperties(
 	filt.Name = newList.Name
 
 	if filt.URL != newList.URL {
-		if d.filterExistsNoLock(newList.URL) {
+		if d.filterExistsIntl(newList.URL) {
 			return false, errFilterExists
 		}
 
-		restart = true
+		shouldRestart = true
 
 		filt.URL = newList.URL
 		filt.LastUpdated = time.Time{}
@@ -122,45 +122,51 @@ func (d *DNSFilter) filterSetProperties(
 
 	if filt.Enabled != newList.Enabled {
 		filt.Enabled = newList.Enabled
-		restart = true
+		shouldRestart = true
 	}
 
 	if filt.Enabled {
-		if restart {
+		if shouldRestart {
 			// Download the filter contents.
-			restart, err = d.update(filt)
+			shouldRestart, err = d.update(filt)
 		}
 	} else {
-		// TODO(e.burkov):  Validating of the new URL's content is skipped if
-		// the filter is turned off.  It makes possible to set the malformed
-		// rules source, but will disallow to enable the filter afterwards.
+		// TODO(e.burkov):  The validation of the contents of the new URL is
+		// currently skipped if the rule list is disabled.  This makes it
+		// possible to set a bad rules source, but the validation should still
+		// kick in when the filter is enabled.  Consider making changing this
+		// behavior to be stricter.
 		filt.unload()
 	}
 
-	return restart, err
+	return shouldRestart, err
 }
 
-// Return TRUE if a filter with this URL exists
-func (d *DNSFilter) filterExists(url string) bool {
+// filterExists returns true if a filter with the same url exists in d.
+func (d *DNSFilter) filterExists(url string) (ok bool) {
 	d.filtersMu.RLock()
 	defer d.filtersMu.RUnlock()
 
-	r := d.filterExistsNoLock(url)
+	r := d.filterExistsIntl(url)
 
 	return r
 }
 
-func (d *DNSFilter) filterExistsNoLock(url string) bool {
+// filterExistsIntl returns true if d contains the filter with the same url.
+// For internal use only.
+func (d *DNSFilter) filterExistsIntl(url string) (ok bool) {
 	for _, f := range d.Filters {
 		if f.URL == url {
 			return true
 		}
 	}
+
 	for _, f := range d.WhitelistFilters {
 		if f.URL == url {
 			return true
 		}
 	}
+
 	return false
 }
 
@@ -171,7 +177,7 @@ func (d *DNSFilter) filterAdd(flt FilterYAML) bool {
 	defer d.filtersMu.Unlock()
 
 	// Check for duplicates
-	if d.filterExistsNoLock(flt.URL) {
+	if d.filterExistsIntl(flt.URL) {
 		return false
 	}
 

internal/filtering/filter_test.go

@@ -4,44 +4,42 @@ import (
 	"io/fs"
 	"net"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"testing"
 	"time"
 
-	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-const testFltsFileName = "1.txt"
-
-// testStartFilterListener is a helper that concurrently listens on a free port
+// serveFiltersLocally is a helper that concurrently listens on a free port
 // to respond with fltContent.
-//
-// TODO(e.burkov):  Remove pointer to the slice of bytes.
-func testStartFilterListener(t *testing.T, fltContent *[]byte) (l net.Listener) {
+func serveFiltersLocally(t *testing.T, fltContent []byte) (ipp netip.AddrPort) {
 	t.Helper()
 
 	h := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		pt := testutil.PanicT{}
 
-		n, werr := w.Write(*fltContent)
+		n, werr := w.Write(fltContent)
 		require.NoError(pt, werr)
-		require.Equal(pt, len(*fltContent), n)
+		require.Equal(pt, len(fltContent), n)
 	})
 
-	var err error
-	l, err = net.Listen("tcp", ":0")
+	l, err := net.Listen("tcp", ":0")
 	require.NoError(t, err)
 
 	go func() { _ = http.Serve(l, h) }()
 	testutil.CleanupAndRequireSuccess(t, l.Close)
 
-	return l
+	addr := l.Addr()
+	require.IsType(t, new(net.TCPAddr), addr)
+
+	return netip.AddrPortFrom(aghnet.IPv4Localhost(), uint16(addr.(*net.TCPAddr).Port))
 }
 
 func TestFilters(t *testing.T) {
@@ -53,7 +51,7 @@ func TestFilters(t *testing.T) {
 
 	fltContent := []byte(content)
 
-	l := testStartFilterListener(t, &fltContent)
+	addr := serveFiltersLocally(t, fltContent)
 
 	tempDir := t.TempDir()
 
@@ -68,11 +66,7 @@ func TestFilters(t *testing.T) {
 	f := &FilterYAML{
 		URL: (&url.URL{
 			Scheme: "http",
-			Host: (&netutil.IPPort{
-				IP:   net.IP{127, 0, 0, 1},
-				Port: l.Addr().(*net.TCPAddr).Port,
-			}).String(),
-			Path: path.Join(filterDir, testFltsFileName),
+			Host:   addr.String(),
 		}).String(),
 	}
 
@@ -105,8 +99,15 @@ func TestFilters(t *testing.T) {
 	})
 
 	t.Run("refresh_actually", func(t *testing.T) {
-		fltContent = []byte(`||example.com^`)
-		t.Cleanup(func() { fltContent = []byte(content) })
+		anotherContent := []byte(`||example.com^`)
+		oldURL := f.URL
+
+		ipp := serveFiltersLocally(t, anotherContent)
+		f.URL = (&url.URL{
+			Scheme: "http",
+			Host:   ipp.String(),
+		}).String()
+		t.Cleanup(func() { f.URL = oldURL })
 
 		updateAndAssert(t, require.True, 1)
 	})

internal/filtering/filtering.go

@@ -345,26 +345,29 @@ func (d *DNSFilter) SetFilters(blockFilters, allowFilters []Filter, async bool)
 			blockFilters: blockFilters,
 		}
 
-		d.filtersInitializerLock.Lock() // prevent multiple writers from adding more than 1 task
+		d.filtersInitializerLock.Lock()
 		defer d.filtersInitializerLock.Unlock()
 
-		// remove all pending tasks
-		for stop := false; !stop; {
+		// Remove all pending tasks.
+	removeLoop:
+		for {
 			select {
 			case <-d.filtersInitializerChan:
-				//
+				// Continue removing.
 			default:
-				stop = true
+				break removeLoop
 			}
 		}
 
 		d.filtersInitializerChan <- params
+
 		return nil
 	}
 
 	err := d.initFiltering(allowFilters, blockFilters)
 	if err != nil {
-		log.Error("Can't initialize filtering subsystem: %s", err)
+		log.Error("can't initialize filtering subsystem: %s", err)
+
 		return err
 	}
 

internal/filtering/http_test.go

@@ -3,15 +3,12 @@ package filtering
 import (
 	"bytes"
 	"encoding/json"
-	"net"
 	"net/http"
 	"net/http/httptest"
-	"net/netip"
 	"net/url"
 	"testing"
 	"time"
 
-	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -33,16 +30,10 @@ func TestDNSFilter_handleFilteringSetURL(t *testing.T) {
 		endpoint: &badRulesEndpoint,
 		content:  []byte(`<html></html>`),
 	}} {
-		rulesSource := rulesSource
-		listener := testStartFilterListener(t, &rulesSource.content)
-
-		addr := listener.Addr()
-		require.IsType(t, new(net.TCPAddr), addr)
-		port := addr.(*net.TCPAddr).AddrPort().Port()
-
+		ipp := serveFiltersLocally(t, rulesSource.content)
 		*rulesSource.endpoint = (&url.URL{
 			Scheme: "http",
-			Host:   netip.AddrPortFrom(aghnet.IPv4Localhost(), port).String(),
+			Host:   ipp.String(),
 		}).String()
 	}
 
@@ -109,17 +100,15 @@ func TestDNSFilter_handleFilteringSetURL(t *testing.T) {
 	}}
 
 	for _, tc := range testCases {
-		tc := tc
-
 		t.Run(tc.name, func(t *testing.T) {
-			configModified := false
+			confModifiedCalled := false
 			d, err := New(&Config{
 				FilteringEnabled: true,
 				Filters:          tc.initial,
 				HTTPClient: &http.Client{
 					Timeout: 5 * time.Second,
 				},
-				ConfigModified: func() { configModified = true },
+				ConfigModified: func() { confModifiedCalled = true },
 				DataDir:        filtersDir,
 			}, nil)
 			require.NoError(t, err)
@@ -145,11 +134,10 @@ func TestDNSFilter_handleFilteringSetURL(t *testing.T) {
 
 			d.handleFilteringSetURL(w, r)
 			assert.Equal(t, tc.wantBody, w.Body.String())
-			if tc.wantBody != "" {
-				assert.False(t, configModified)
-			} else {
-				assert.True(t, configModified)
-			}
+
+			// For the moment the non-empty response body only contains occurred
+			// error, so the configuration shouldn't be written.
+			assert.Equal(t, tc.wantBody == "", confModifiedCalled)
 		})
 	}
 }