refactor(kbhttp): one reconciling goroutine for both local servers

go/chat/attachment_httpsrv.go

@@ -121,13 +121,11 @@ func (r *AttachmentHTTPSrv) genURLKey(prefix string, payload any) (string, error
 }
 
 func (r *AttachmentHTTPSrv) getURL(ctx context.Context, prefix string, payload any) string {
-	if !r.httpSrv.Active() {
-		r.Debug(ctx, "getURL: http server failed to start earlier")
-		return ""
-	}
+	// Addr fails only before the server first binds; while it is stopped it
+	// returns where the server comes back.
 	addr, err := r.httpSrv.Addr()
 	if err != nil {
-		r.Debug(ctx, "getURL: failed to get HTTP server address: %s", err)
+		r.Debug(ctx, "getURL: no HTTP server address: %s", err)
 		return ""
 	}
 	key, err := r.genURLKey(prefix, payload)
@@ -149,6 +147,10 @@ func (r *AttachmentHTTPSrv) GetURL(ctx context.Context, convID chat1.Conversatio
 		ConvID: convID,
 		MsgID:  msgID,
 	})
+	if url == "" {
+		// Without a server there is no URL; the query alone would be a garbage one.
+		return ""
+	}
 	url += fmt.Sprintf("&prev=%v&noanim=%v&isemoji=%v", preview, noAnim, isEmoji)
 	r.Debug(ctx, "GetURL: handler URL: convID: %s msgID: %d %s", convID, msgID, url)
 	return url

go/chat/attachment_httpsrv_appstate_test.go

@@ -0,0 +1,90 @@
+package chat
+
+import (
+	"context"
+	"net"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/keybase/client/go/chat/globals"
+	"github.com/keybase/client/go/chat/types"
+	"github.com/keybase/client/go/externalstest"
+	"github.com/keybase/client/go/kbhttp/manager"
+	"github.com/keybase/client/go/libkb"
+	"github.com/keybase/client/go/protocol/chat1"
+	"github.com/keybase/client/go/protocol/keybase1"
+	"github.com/stretchr/testify/require"
+)
+
+type startOnlyAttachmentFetcher struct {
+	types.AttachmentFetcher
+}
+
+func (startOnlyAttachmentFetcher) OnStart(libkb.MetaContext) {}
+
+// requireSrvServing waits until the server does or does not accept connections
+// at the address it hands out.
+func requireSrvServing(t *testing.T, srv *manager.Srv, serving bool) {
+	t.Helper()
+	require.Eventually(t, func() bool {
+		addr, err := srv.Addr()
+		if err != nil {
+			return false
+		}
+		conn, err := net.DialTimeout("tcp", addr, time.Second)
+		if err == nil {
+			conn.Close()
+		}
+		return (err == nil) == serving
+	}, 10*time.Second, time.Millisecond, "server serving != %v", serving)
+}
+
+func TestGetURLWhileStoppedUsesLastAddress(t *testing.T) {
+	tc := externalstest.SetupTest(t, "attachment-url-stopped", 0)
+	defer tc.Cleanup()
+	tc.G.ConnectionManager = libkb.NewConnectionManager()
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_FOREGROUND)
+	g := globals.NewContext(tc.G, &globals.ChatContext{})
+	httpSrv := manager.NewSrv(tc.G)
+	srv := NewAttachmentHTTPSrv(g, httpSrv, startOnlyAttachmentFetcher{}, nil)
+	g.AttachmentURLSrv = srv
+	emoji := NewDevConvEmojiSource(g, nil)
+	ctx := context.TODO()
+	convID := chat1.ConversationID([]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16})
+	msg := chat1.EmojiMessage{ConvID: convID, MsgID: 3}
+
+	type urls struct {
+		full, preview, emoji, emojiNoAnim, emojiNoAnimOnly string
+	}
+	get := func() urls {
+		var res urls
+		res.full = srv.GetURL(ctx, msg.ConvID, msg.MsgID, false, false, false)
+		res.preview = srv.GetURL(ctx, msg.ConvID, msg.MsgID, true, false, false)
+		source, noAnimSource, err := emoji.RemoteToLocalSource(ctx, chat1.NewEmojiRemoteSourceWithMessage(msg), false)
+		require.NoError(t, err)
+		res.emoji, res.emojiNoAnim = source.Httpsrv(), noAnimSource.Httpsrv()
+		source, _, err = emoji.RemoteToLocalSource(ctx, chat1.NewEmojiRemoteSourceWithMessage(msg), true)
+		require.NoError(t, err)
+		res.emojiNoAnimOnly = source.Httpsrv()
+		return res
+	}
+
+	requireSrvServing(t, httpSrv, true)
+	addr, err := httpSrv.Addr()
+	require.NoError(t, err)
+	prefix := "http://" + addr + "/"
+	up := get()
+	for _, url := range []string{up.full, up.preview, up.emoji, up.emojiNoAnim, up.emojiNoAnimOnly} {
+		require.True(t, strings.HasPrefix(url, prefix), "url %q while serving", url)
+	}
+	require.Contains(t, up.preview, "&prev=true")
+
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_BACKGROUND)
+	requireSrvServing(t, httpSrv, false)
+	require.Equal(t, up, get())
+
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_FOREGROUND)
+	requireSrvServing(t, httpSrv, true)
+	require.Equal(t, up, get())
+}

go/kbfs/libhttpserver/server.go

@@ -8,10 +8,10 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/base64"
-	"errors"
 	"io"
 	"net/http"
 	"path"
+	"runtime"
 	"strings"
 	"sync"
 	"time"
@@ -24,6 +24,7 @@ import (
 	"github.com/keybase/client/go/kbfs/libmime"
 	"github.com/keybase/client/go/kbfs/tlf"
 	"github.com/keybase/client/go/kbhttp"
+	"github.com/keybase/client/go/kbhttp/manager"
 	"github.com/keybase/client/go/libkb"
 	"github.com/keybase/client/go/logger"
 	"github.com/keybase/client/go/protocol/keybase1"
@@ -33,20 +34,17 @@ const fsCacheSize = 64
 
 // Server is a local HTTP server for serving KBFS content over HTTP.
 type Server struct {
-	config          libkbfs.Config
-	logger          logger.Logger
-	vlog            *libkb.VDebugLog
-	appStateUpdater env.AppStateUpdater
-	cancel          func()
+	config libkbfs.Config
+	logger logger.Logger
+	vlog   *libkb.VDebugLog
 
 	tokenLock       sync.RWMutex
 	token           string
 	tokenExpireTime time.Time
 
 	fs *lru.Cache
 
-	serverLock sync.RWMutex
-	server     *kbhttp.Srv
+	server *manager.Srv
 }
 
 const (
@@ -221,67 +219,15 @@ const (
 	requestPathRoot = "/files/"
 )
 
-func (s *Server) restart() (err error) {
-	s.serverLock.Lock()
-	defer s.serverLock.Unlock()
-	if s.server != nil {
-		s.server.Stop()
-		err = s.server.Start()
-	}
-	if s.server == nil ||
-		// If pinned port is in use, just pick a new one like we never had a
-		// server before.
-		errors.Is(err, kbhttp.ErrPinnedPortInUse) {
-		s.server = kbhttp.NewSrv(s.logger,
-			kbhttp.NewRandomPortRangeListenerSource(portStart, portEnd))
-		err = s.server.Start()
-	}
-	if err != nil {
-		return err
-	}
-	// Have to start this first to populate the ServeMux object.
-	s.server.Handle(requestPathRoot,
-		http.StripPrefix(requestPathRoot, http.HandlerFunc(s.serve)))
-	return nil
-}
-
-func (s *Server) monitorAppState(ctx context.Context) {
-	state := keybase1.MobileAppState_FOREGROUND
-	for {
-		select {
-		case <-ctx.Done():
-			return
-		case <-s.appStateUpdater.NextAppStateUpdate(state):
-			state = s.appStateUpdater.AppState()
-			// Due to the way NextUpdate is designed, it's possible we miss an
-			// update if processing the last update takes too long. So it's
-			// possible to get consecutive FOREGROUND updates even if there are
-			// other states in-between. Since libkb/appstate.go already
-			// deduplicates, it'll never actually send consecutive identical
-			// states to us. In addition, apart from FOREGROUND/BACKGROUND,
-			// there are other possible states too, and potentially more in the
-			// future. So, we just restart the server under FOREGROUND instead
-			// of trying to listen on all state updates.
-			if state != keybase1.MobileAppState_FOREGROUND {
-				continue
-			}
-			if err := s.restart(); err != nil {
-				s.logger.Error("(Re)starting server failed: %v", err)
-			}
-		}
-	}
-}
-
 // New creates and starts a new server.
 func New(appStateUpdater env.AppStateUpdater, config libkbfs.Config) (
 	s *Server, err error,
 ) {
 	logger := config.MakeLogger("HTTP")
 	s = &Server{
-		appStateUpdater: appStateUpdater,
-		config:          config,
-		logger:          logger,
-		vlog:            config.MakeVLogger(logger),
+		config: config,
+		logger: logger,
+		vlog:   config.MakeVLogger(logger),
 	}
 	s.fs, err = lru.NewWithEvict(fsCacheSize, func(_ any, value any) {
 		if e, ok := value.(obsoleteTrackingFS); ok && e.unsubscribe != nil {
@@ -291,30 +237,33 @@ func New(appStateUpdater env.AppStateUpdater, config libkbfs.Config) (
 	if err != nil {
 		return nil, err
 	}
-	if err = s.restart(); err != nil {
+	// A failed first start is fatal here: the retry rides on app state changes,
+	// and on desktop -- which runs this server too -- the app state never moves.
+	s.server, err = manager.New("kbfsHTTP", logger, appStateUpdater.AppState, appStateUpdater.NextAppStateUpdate,
+		func() kbhttp.ListenerSource {
+			return kbhttp.NewRandomPortRangeListenerSource(portStart, portEnd)
+		}, runtime.GOOS != "android", func(context.Context, keybase1.HttpSrvInfo) {})
+	if err != nil {
+		s.server.Shutdown()
 		return nil, err
 	}
-	ctx, cancel := context.WithCancel(context.Background())
-	go s.monitorAppState(ctx)
-	s.cancel = cancel
+	// The token is checked in serve. No one has the address before New
+	// returns, so registering after the first start answers no request with a 404.
+	s.server.HandleFunc(strings.TrimPrefix(requestPathRoot, "/"), manager.SrvTokenModeUnchecked,
+		http.StripPrefix(requestPathRoot, http.HandlerFunc(s.serve)).ServeHTTP)
 	libmime.Patch(additionalMimeTypes)
 	return s, nil
 }
 
 // Address returns the address that the server is listening on.
 func (s *Server) Address() (string, error) {
-	s.serverLock.RLock()
-	defer s.serverLock.RUnlock()
 	return s.server.Addr()
 }
 
 // Shutdown shuts down the server.
 func (s *Server) Shutdown() {
-	s.serverLock.Lock()
-	defer s.serverLock.Unlock()
-	s.server.Stop()
+	s.server.Shutdown()
 	// Purge the LRU so its evict callback runs and unsubscribes any
 	// folder-branch observers still held by cached entries.
 	s.fs.Purge()
-	s.cancel()
 }