portlist: move sync.Once up and Close on err

Signed-off-by: Marwan Sulaiman <marwan@tailscale.com>

ipn/ipnlocal/local.go

@@ -292,9 +292,12 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	osshare.SetFileSharingEnabled(false, logf)
 
 	ctx, cancel := context.WithCancel(context.Background())
-	portpoll, err := portlist.NewPoller()
+	portpoll := new(portlist.Poller)
+	err = portpoll.Check()
 	if err != nil {
 		logf("skipping portlist: %s", err)
+		portpoll.Close()
+		portpoll = nil
 	}
 
 	b := &LocalBackend{

portlist/poller.go

@@ -29,7 +29,8 @@ type Poller struct {
 	// This field should only be changed before calling Run.
 	IncludeLocalhost bool
 
-	c chan List // unbuffered
+	initOnce sync.Once // guards init of private fields
+	c        chan List // unbuffered
 
 	// os, if non-nil, is an OS-specific implementation of the portlist getting
 	// code. When non-nil, it's responsible for getting the complete list of
@@ -37,8 +38,7 @@ type Poller struct {
 	// addProcesses is not used.
 	// A nil values means we don't have code for getting the list on the current
 	// operating system.
-	os     osImpl
-	osOnce sync.Once // guards init of os
+	os osImpl
 
 	// closeCtx is the context that's canceled on Close.
 	closeCtx       context.Context
@@ -71,54 +71,44 @@ var newOSImpl func(includeLocalhost bool) osImpl
 
 var errUnimplemented = errors.New("portlist poller not implemented on " + runtime.GOOS)
 
-// NewPoller returns a new portlist Poller. It returns an error
-// if the portlist couldn't be obtained.
-func NewPoller() (*Poller, error) {
-	if debugDisablePortlist() {
-		return nil, errors.New("portlist disabled by envknob")
-	}
-	p := &Poller{
-		c:       make(chan List),
-		runDone: make(chan struct{}),
-	}
-	p.closeCtx, p.closeCtxCancel = context.WithCancel(context.Background())
-	p.osOnce.Do(p.initOSField)
-	if p.os == nil {
-		return nil, errUnimplemented
-	}
-
-	// Do one initial poll synchronously so we can return an error
-	// early.
-	if pl, err := p.getList(); err != nil {
-		return nil, err
-	} else {
-		p.setPrev(pl)
-	}
-	return p, nil
-}
-
 func (p *Poller) setPrev(pl List) {
 	// Make a copy, as the pass in pl slice aliases pl.scratch and we don't want
 	// that to except to the caller.
 	p.prev = slices.Clone(pl)
 }
 
-func (p *Poller) initOSField() {
+// init sets the os implementation if exists. It also sets
+// all private fields. All exported methods must call this in a
+// Once, otherwise they may panic.
+func (p *Poller) init() {
+	if debugDisablePortlist() {
+		return
+	}
 	if newOSImpl != nil {
 		p.os = newOSImpl(p.IncludeLocalhost)
 	}
+	p.closeCtx, p.closeCtxCancel = context.WithCancel(context.Background())
+	p.c = make(chan List)
+	p.runDone = make(chan struct{})
 }
 
 // Updates return the channel that receives port list updates.
 //
 // The channel is closed when the Poller is closed.
-func (p *Poller) Updates() <-chan List { return p.c }
+func (p *Poller) Updates() <-chan List {
+	p.initOnce.Do(p.init)
+	return p.c
+}
 
 // Close closes the Poller.
 // Run will return with a nil error.
 func (p *Poller) Close() error {
+	p.initOnce.Do(p.init)
 	p.closeCtxCancel()
-	<-p.runDone
+	if p.os == nil {
+		return nil
+	}
+	<-p.runDone // if caller of Close never called Run, this can hang.
 	if p.os != nil {
 		p.os.Close()
 	}
@@ -147,7 +137,33 @@ func (p *Poller) Run(ctx context.Context) error {
 	return p.runWithTickChan(ctx, tick.C)
 }
 
+// Check makes sure that the Poller is enabled and
+// the undelrying OS implementation is working properly.
+//
+// An error returned from Check is non-fatal and means
+// that it's been administratively disabled or the underlying
+// OS is not implemented.
+func (p *Poller) Check() error {
+	p.initOnce.Do(p.init)
+	if p.os == nil {
+		return errUnimplemented
+	}
+	// Do one initial poll synchronously so we can return an error
+	// early.
+	if pl, err := p.getList(); err != nil {
+		return err
+	} else {
+		p.setPrev(pl)
+	}
+	return nil
+}
+
 func (p *Poller) runWithTickChan(ctx context.Context, tickChan <-chan time.Time) error {
+	p.initOnce.Do(p.init)
+	if p.os == nil {
+		return errUnimplemented
+	}
+
 	defer close(p.runDone)
 	defer close(p.c)
 
@@ -182,7 +198,7 @@ func (p *Poller) getList() (List, error) {
 	if debugDisablePortlist() {
 		return nil, nil
 	}
-	p.osOnce.Do(p.initOSField)
+	p.initOnce.Do(p.init)
 	var err error
 	p.scratch, err = p.os.AppendListeningPorts(p.scratch[:0])
 	return p.scratch, err

portlist/portlist_test.go

@@ -176,7 +176,8 @@ func TestEqualLessThan(t *testing.T) {
 }
 
 func TestPoller(t *testing.T) {
-	p, err := NewPoller()
+	var p Poller
+	err := p.Check()
 	if err != nil {
 		t.Skipf("not running test: %v", err)
 	}