portlist: remove NewPoller constructor

This is a follow up on PR #8172 and a breaking change that removes NewPoller.
The issue with the previous PR was that NewPoller immediately initializes the underlying os implementation
and therefore setting IncludeLocalhost as an exported field happened too late and cannot happen early enough.
Using the zero value of Poller was also not an option from outside of the package because we need to set initial
private fields

Fixes #8171

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

ipn/ipnlocal/local.go

@@ -292,9 +292,11 @@ 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 = nil
 	}
 
 	b := &LocalBackend{

portlist/poller.go

@@ -31,14 +31,15 @@ type Poller struct {
 
 	c chan List // unbuffered
 
+	initOnce sync.Once // guards init of private fields
+
 	// 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
 	// cached ports complete with the process name. That is, when set,
 	// 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 +72,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 +138,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 +199,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)
 	}