Pull request 1855: fix-clients-races

Merge in DNS/adguard-home from fix-clients-races to master

Squashed commit of the following:

commit 2b29271b4c04b1c3046bd4cb3fb82f82fff0973b
Merge: 7f553feff cbc7985e7
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu May 25 17:43:07 2023 +0300

    Merge branch 'master' into fix-clients-races

commit 7f553feff3aa46ae124a984776b60ed625148a1f
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu May 25 16:25:43 2023 +0300

    home: imp docs more

commit aaf74666cf069fc6cdd5a5e64915fef5675e0382
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu May 25 13:57:42 2023 +0300

    home: imp docs

commit 50085085da3ae07b58bcc471906725339eec353e
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed May 24 15:41:52 2023 +0300

    home: imp code more

commit 9dc001640ec4bef62b2ccaed307a9daeeb3a8d3a
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Tue May 23 15:30:53 2023 +0300

    home: imp code

commit fa906e78ec0777c8c585622873addd0f4791554e
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Tue May 23 14:17:16 2023 +0300

    home: fix clients races

internal/home/client.go

@@ -8,6 +8,7 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering/safesearch"
 	"github.com/AdguardTeam/dnsproxy/proxy"
+	"github.com/AdguardTeam/golibs/stringutil"
 )
 
 // Client contains information about persistent clients.
@@ -37,6 +38,19 @@ type Client struct {
 	IgnoreStatistics      bool
 }
 
+// ShallowClone returns a deep copy of the client, except upstreamConfig,
+// safeSearchConf, SafeSearch fields, because it's difficult to copy them.
+func (c *Client) ShallowClone() (sh *Client) {
+	clone := *c
+
+	clone.IDs = stringutil.CloneSlice(c.IDs)
+	clone.Tags = stringutil.CloneSlice(c.Tags)
+	clone.BlockedServices = stringutil.CloneSlice(c.BlockedServices)
+	clone.Upstreams = stringutil.CloneSlice(c.Upstreams)
+
+	return &clone
+}
+
 // closeUpstreams closes the client-specific upstream config of c if any.
 func (c *Client) closeUpstreams() (err error) {
 	if c.upstreamConfig != nil {

internal/home/clients.go

@@ -378,6 +378,7 @@ func (clients *clientsContainer) clientOrArtificial(
 	}, true
 }
 
+// Find returns a shallow copy of the client if there is one found.
 func (clients *clientsContainer) Find(id string) (c *Client, ok bool) {
 	clients.lock.Lock()
 	defer clients.lock.Unlock()
@@ -387,20 +388,18 @@ func (clients *clientsContainer) Find(id string) (c *Client, ok bool) {
 		return nil, false
 	}
 
-	c.IDs = stringutil.CloneSlice(c.IDs)
+	return c.ShallowClone(), true
-	c.Tags = stringutil.CloneSlice(c.Tags)
-	c.BlockedServices = stringutil.CloneSlice(c.BlockedServices)
-	c.Upstreams = stringutil.CloneSlice(c.Upstreams)
-
-	return c, true
 }
 
 // shouldCountClient is a wrapper around Find to make it a valid client
 // information finder for the statistics.  If no information about the client
 // is found, it returns true.
 func (clients *clientsContainer) shouldCountClient(ids []string) (y bool) {
+	clients.lock.Lock()
+	defer clients.lock.Unlock()
+
 	for _, id := range ids {
-		client, ok := clients.Find(id)
+		client, ok := clients.findLocked(id)
 		if ok {
 			return !client.IgnoreStatistics
 		}
@@ -617,6 +616,15 @@ func (clients *clientsContainer) Add(c *Client) (ok bool, err error) {
 		}
 	}
 
+	clients.add(c)
+
+	log.Debug("clients: added %q: ID:%q [%d]", c.Name, c.IDs, len(clients.list))
+
+	return true, nil
+}
+
+// add c to the indexes. clients.lock is expected to be locked.
+func (clients *clientsContainer) add(c *Client) {
 	// update Name index
 	clients.list[c.Name] = c
 
@@ -624,10 +632,6 @@ func (clients *clientsContainer) Add(c *Client) (ok bool, err error) {
 	for _, id := range c.IDs {
 		clients.idIndex[id] = c
 	}
-
-	log.Debug("clients: added %q: ID:%q [%d]", c.Name, c.IDs, len(clients.list))
-
-	return true, nil
 }
 
 // Del removes a client.  ok is false if there is no such client.
@@ -645,86 +649,53 @@ func (clients *clientsContainer) Del(name string) (ok bool) {
 		log.Error("client container: removing client %s: %s", name, err)
 	}
 
+	clients.del(c)
+
+	return true
+}
+
+// del removes c from the indexes. clients.lock is expected to be locked.
+func (clients *clientsContainer) del(c *Client) {
 	// update Name index
-	delete(clients.list, name)
+	delete(clients.list, c.Name)
 
 	// update ID index
 	for _, id := range c.IDs {
 		delete(clients.idIndex, id)
 	}
-
-	return true
 }
 
 // Update updates a client by its name.
-func (clients *clientsContainer) Update(name string, c *Client) (err error) {
+func (clients *clientsContainer) Update(prev, c *Client) (err error) {
 	err = clients.check(c)
 	if err != nil {
+		// Don't wrap the error since it's informative enough as is.
 		return err
 	}
 
 	clients.lock.Lock()
 	defer clients.lock.Unlock()
 
-	prev, ok := clients.list[name]
+	// Check the name index.
-	if !ok {
-		return errors.Error("client not found")
-	}
-
-	// First, check the name index.
 	if prev.Name != c.Name {
-		_, ok = clients.list[c.Name]
+		_, ok := clients.list[c.Name]
 		if ok {
 			return errors.Error("client already exists")
 		}
 	}
 
-	// Second, update the ID index.
+	// Check the ID index.
-	err = clients.updateIDIndex(prev, c.IDs)
+	if !slices.Equal(prev.IDs, c.IDs) {
-	if err != nil {
+		for _, id := range c.IDs {
-		// Don't wrap the error, because it's informative enough as is.
+			existing, ok := clients.idIndex[id]
-		return err
+			if ok && existing != prev {
-	}
+				return fmt.Errorf("id %q is used by client with name %q", id, existing.Name)
-
+			}
-	// Update name index.
-	if prev.Name != c.Name {
-		delete(clients.list, prev.Name)
-		clients.list[c.Name] = prev
-	}
-
-	// Update upstreams cache.
-	err = c.closeUpstreams()
-	if err != nil {
-		return err
-	}
-
-	*prev = *c
-
-	return nil
-}
-
-// updateIDIndex updates the ID index data for cli using the information from
-// newIDs.
-func (clients *clientsContainer) updateIDIndex(cli *Client, newIDs []string) (err error) {
-	if slices.Equal(cli.IDs, newIDs) {
-		return nil
-	}
-
-	for _, id := range newIDs {
-		existing, ok := clients.idIndex[id]
-		if ok && existing != cli {
-			return fmt.Errorf("id %q is used by client with name %q", id, existing.Name)
 		}
 	}
 
-	// Update the IDs in the index.
+	clients.del(prev)
-	for _, id := range cli.IDs {
+	clients.add(c)
-		delete(clients.idIndex, id)
-	}
-
-	for _, id := range newIDs {
-		clients.idIndex[id] = cli
-	}
 
 	return nil
 }

internal/home/clients_test.go

@@ -98,22 +98,8 @@ func TestClients(t *testing.T) {
 		assert.False(t, ok)
 	})
 
-	t.Run("update_fail_name", func(t *testing.T) {
-		err := clients.Update("client3", &Client{
-			IDs:  []string{"1.2.3.0"},
-			Name: "client3",
-		})
-		require.Error(t, err)
-
-		err = clients.Update("client3", &Client{
-			IDs:  []string{"1.2.3.0"},
-			Name: "client2",
-		})
-		assert.Error(t, err)
-	})
-
 	t.Run("update_fail_ip", func(t *testing.T) {
-		err := clients.Update("client1", &Client{
+		err := clients.Update(&Client{Name: "client1"}, &Client{
 			IDs:  []string{"2.2.2.2"},
 			Name: "client1",
 		})
@@ -129,7 +115,10 @@ func TestClients(t *testing.T) {
 			cliNewIP = netip.MustParseAddr(cliNew)
 		)
 
-		err := clients.Update("client1", &Client{
+		prev, ok := clients.list["client1"]
+		require.True(t, ok)
+
+		err := clients.Update(prev, &Client{
 			IDs:  []string{cliNew},
 			Name: "client1",
 		})
@@ -138,7 +127,10 @@ func TestClients(t *testing.T) {
 		assert.Equal(t, clients.clientSource(cliOldIP), ClientSourceNone)
 		assert.Equal(t, clients.clientSource(cliNewIP), ClientSourcePersistent)
 
-		err = clients.Update("client1", &Client{
+		prev, ok = clients.list["client1"]
+		require.True(t, ok)
+
+		err = clients.Update(prev, &Client{
 			IDs:            []string{cliNew},
 			Name:           "client1-renamed",
 			UseOwnSettings: true,

internal/home/clientshttp.go

@@ -289,7 +289,7 @@ func (clients *clientsContainer) handleUpdateClient(w http.ResponseWriter, r *ht
 		return
 	}
 
-	err = clients.Update(dj.Name, c)
+	err = clients.Update(prev, c)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)