Pull request: home: rm unnecessary locking in update; refactor

Merge in DNS/adguard-home from 4499-rm-unnecessary-locking to master

Squashed commit of the following:

commit 6d70472506dd0fd69225454c73d9f7f6a208b76b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 25 17:26:54 2022 +0300

    home: rm unnecessary locking in update; refactor

CHANGELOG.md

@@ -89,6 +89,7 @@ In this release, the schema version has changed from 12 to 13.
 
 ### Fixed
 
+- Slow version update queries making other HTTP APIs unresponsible ([#4499]).
 - ARP tables refreshing process causing excessive PTR requests ([#3157]).
 
 [#1730]: https://github.com/AdguardTeam/AdGuardHome/issues/1730
@@ -106,6 +107,7 @@ In this release, the schema version has changed from 12 to 13.
 [#4221]: https://github.com/AdguardTeam/AdGuardHome/issues/4221
 [#4238]: https://github.com/AdguardTeam/AdGuardHome/issues/4238
 [#4276]: https://github.com/AdguardTeam/AdGuardHome/issues/4276
+[#4499]: https://github.com/AdguardTeam/AdGuardHome/issues/4499
 
 [repr]:         https://reproducible-builds.org/docs/source-date-epoch/
 [doq-draft-10]: https://datatracker.ietf.org/doc/html/draft-ietf-dprive-dnsoquic-10#section-10.2

internal/aghnet/net_bsd.go

@@ -1,5 +1,5 @@
-//go:build !linux
+//go:build darwin || freebsd || openbsd
-// +build !linux
+// +build darwin freebsd openbsd
 
 package aghnet
 

internal/aghnet/net_windows.go

@@ -1,5 +1,5 @@
-//go:build !(linux || darwin || freebsd || openbsd)
+//go:build windows
-// +build !linux,!darwin,!freebsd,!openbsd
+// +build windows
 
 package aghnet
 
@@ -13,6 +13,10 @@ import (
 	"golang.org/x/sys/windows"
 )
 
+func canBindPrivilegedPorts() (can bool, err error) {
+	return true, nil
+}
+
 func ifaceHasStaticIP(string) (ok bool, err error) {
 	return false, aghos.Unsupported("checking static ip")
 }

internal/home/controlupdate.go

@@ -3,6 +3,7 @@ package home
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"os"
 	"os/exec"
@@ -27,12 +28,16 @@ type temporaryError interface {
 
 // Get the latest available version from the Internet
 func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) {
+	w.Header().Set("Content-Type", "application/json")
+
 	resp := &versionResponse{}
 	if Context.disableUpdate {
-		// w.Header().Set("Content-Type", "application/json")
 		resp.Disabled = true
-		_ = json.NewEncoder(w).Encode(resp)
+		err := json.NewEncoder(w).Encode(resp)
-		// TODO(e.burkov): Add error handling and deal with headers.
+		if err != nil {
+			aghhttp.Error(r, w, http.StatusInternalServerError, "writing body: %s", err)
+		}
+
 		return
 	}
 
@@ -44,30 +49,48 @@ func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) {
 	if r.ContentLength != 0 {
 		err = json.NewDecoder(r.Body).Decode(req)
 		if err != nil {
-			aghhttp.Error(r, w, http.StatusBadRequest, "JSON parse: %s", err)
+			aghhttp.Error(r, w, http.StatusBadRequest, "parsing request: %s", err)
 
 			return
 		}
 	}
 
+	err = requestVersionInfo(resp, req.Recheck)
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		aghhttp.Error(r, w, http.StatusBadGateway, "%s", err)
+
+		return
+	}
+
+	err = resp.setAllowedToAutoUpdate()
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err)
+
+		return
+	}
+
+	err = json.NewEncoder(w).Encode(resp)
+	if err != nil {
+		aghhttp.Error(r, w, http.StatusInternalServerError, "writing body: %s", err)
+	}
+}
+
+// requestVersionInfo sets the VersionInfo field of resp if it can reach the
+// update server.
+func requestVersionInfo(resp *versionResponse, recheck bool) (err error) {
 	for i := 0; i != 3; i++ {
-		func() {
+		resp.VersionInfo, err = Context.updater.VersionInfo(recheck)
-			Context.controlLock.Lock()
-			defer Context.controlLock.Unlock()
-
-			resp.VersionInfo, err = Context.updater.VersionInfo(req.Recheck)
-		}()
-
 		if err != nil {
 			var terr temporaryError
 			if errors.As(err, &terr) && terr.Temporary() {
-				// Temporary network error.  This case may happen while
+				// Temporary network error.  This case may happen while we're
-				// we're restarting our DNS server.  Log and sleep for
+				// restarting our DNS server.  Log and sleep for some time.
-				// some time.
 				//
 				// See https://github.com/AdguardTeam/AdGuardHome/issues/934.
 				d := time.Duration(i) * time.Second
-				log.Info("temp net error: %q; sleeping for %s and retrying", err, d)
+				log.Info("update: temp net error: %q; sleeping for %s and retrying", err, d)
 				time.Sleep(d)
 
 				continue
@@ -76,29 +99,14 @@ func handleGetVersionJSON(w http.ResponseWriter, r *http.Request) {
 
 		break
 	}
+
 	if err != nil {
 		vcu := Context.updater.VersionCheckURL()
-		// TODO(a.garipov): Figure out the purpose of %T verb.
-		aghhttp.Error(
-			r,
-			w,
-			http.StatusBadGateway,
-			"Couldn't get version check json from %s: %T %s\n",
-			vcu,
-			err,
-			err,
-		)
 
-		return
+		return fmt.Errorf("getting version info from %s: %s", vcu, err)
 	}
 
-	resp.confirmAutoUpdate()
+	return nil
-
-	w.Header().Set("Content-Type", "application/json")
-	err = json.NewEncoder(w).Encode(resp)
-	if err != nil {
-		aghhttp.Error(r, w, http.StatusInternalServerError, "Couldn't write body: %s", err)
-	}
 }
 
 // handleUpdate performs an update to the latest available version procedure.
@@ -132,31 +140,37 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) {
 
 // versionResponse is the response for /control/version.json endpoint.
 type versionResponse struct {
-	Disabled bool `json:"disabled"`
 	updater.VersionInfo
+	Disabled bool `json:"disabled"`
 }
 
-// confirmAutoUpdate checks the real possibility of auto update.
+// setAllowedToAutoUpdate sets CanAutoUpdate to true if AdGuard Home is actually
-func (vr *versionResponse) confirmAutoUpdate() {
+// allowed to perform an automatic update by the OS.
-	if vr.CanAutoUpdate != nil && *vr.CanAutoUpdate {
+func (vr *versionResponse) setAllowedToAutoUpdate() (err error) {
-		canUpdate := true
+	if vr.CanAutoUpdate == nil || !*vr.CanAutoUpdate {
+		return
+	}
 
-		var tlsConf *tlsConfigSettings
+	tlsConf := &tlsConfigSettings{}
-		if runtime.GOOS != "windows" {
-			tlsConf = &tlsConfigSettings{}
 	Context.tls.WriteDiskConfig(tlsConf)
+
+	canUpdate := true
+	if tlsConfUsesPrivilegedPorts(tlsConf) || config.BindPort < 1024 || config.DNS.Port < 1024 {
+		canUpdate, err = aghnet.CanBindPrivilegedPorts()
+		if err != nil {
+			return fmt.Errorf("checking ability to bind privileged ports: %w", err)
+		}
 	}
 
-		if tlsConf != nil &&
-			((tlsConf.Enabled && (tlsConf.PortHTTPS < 1024 ||
-				tlsConf.PortDNSOverTLS < 1024 ||
-				tlsConf.PortDNSOverQUIC < 1024)) ||
-				config.BindPort < 1024 ||
-				config.DNS.Port < 1024) {
-			canUpdate, _ = aghnet.CanBindPrivilegedPorts()
-		}
 	vr.CanAutoUpdate = &canUpdate
-	}
+
+	return nil
+}
+
+// tlsConfUsesPrivilegedPorts returns true if the provided TLS configuration
+// indicates that privileged ports are used.
+func tlsConfUsesPrivilegedPorts(c *tlsConfigSettings) (ok bool) {
+	return c.Enabled && (c.PortHTTPS < 1024 || c.PortDNSOverTLS < 1024 || c.PortDNSOverQUIC < 1024)
 }
 
 // finishUpdate completes an update procedure.

internal/updater/check.go

@@ -17,11 +17,11 @@ const versionCheckPeriod = 8 * time.Hour
 
 // VersionInfo contains information about a new version.
 type VersionInfo struct {
+	CanAutoUpdate        *bool  `json:"can_autoupdate,omitempty"`
 	NewVersion           string `json:"new_version,omitempty"`
 	Announcement         string `json:"announcement,omitempty"`
 	AnnouncementURL      string `json:"announcement_url,omitempty"`
 	SelfUpdateMinVersion string `json:"-"`
-	CanAutoUpdate        *bool  `json:"can_autoupdate,omitempty"`
 }
 
 // MaxResponseSize is responses on server's requests maximum length in bytes.

scripts/make/go-lint.sh

@@ -136,7 +136,6 @@ underscores() {
 			-e '_freebsd.go'\
 			-e '_linux.go'\
 			-e '_little.go'\
-			-e '_nolinux.go'\
 			-e '_openbsd.go'\
 			-e '_others.go'\
 			-e '_test.go'\