holepunch: s.holePuncher is nil on close

[watjurk]

Sys info

go-libp2p@v0.19.0

Issue

When closing holepuncher it panics when it hadn't been created yet. Let me explain:

When holepuncher is closed, then the s.holePuncher is closed no matter what:

go-libp2p/p2p/protocol/holepunch/svc.go

Lines 148 to 156 in 5151d4b

	// Close closes the Hole Punch Service.
	func (s *Service) Close() error {
	err := s.holePuncher.Close()
	s.tracer.Close()
	s.host.RemoveStreamHandler(Protocol)
	s.ctxCancel()
	s.refCount.Wait()
	return err
	}

It's worth noting that s.holePuncher is a pointer that is nil by default:

go-libp2p/p2p/protocol/holepunch/svc.go

Lines 44 to 57 in 5151d4b

	type Service struct {
	ctx context.Context
	ctxCancel context.CancelFunc
	host host.Host
	ids identify.IDService
	holePuncher *holePuncher
	hasPublicAddrsChan chan struct{}
	tracer *tracer
	refCount sync.WaitGroup
	}

And that s.holePuncher is only initialized in s.watchForPublicAddr func when some conditions are met:

go-libp2p/p2p/protocol/holepunch/svc.go

Line 141 in 5151d4b

s.holePuncher = newHolePuncher(s.host, s.ids, s.tracer)

This is a problem because if the s.holePuncher isn't initalized and we would call s.Close then s.holePuncher.Close() would panic (because it is not designed to be "zero value" closed).

Solution:

The simplest solution is to introduce a nil check when closing holepuncher, it will check if s.holePuncher is nil or not.

[Stebalien]

Hm. This is also a race-condition.

[watjurk]

Where is the race condition happening? I've run this code with --race enabled and no races were found.

[marten-seemann]

s.holePuncher might be set just when you call Close. We might need to introduce a mutex here.