Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autonat: Clean up after close #2749

Merged
merged 2 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/libp2p/go-libp2p/p2p/host/autonat"
"github.com/libp2p/go-libp2p/p2p/host/autorelay"
bhost "github.com/libp2p/go-libp2p/p2p/host/basic"
blankhost "github.com/libp2p/go-libp2p/p2p/host/blank"
"github.com/libp2p/go-libp2p/p2p/host/eventbus"
"github.com/libp2p/go-libp2p/p2p/host/peerstore/pstoremem"
rcmgr "github.com/libp2p/go-libp2p/p2p/host/resource-manager"
Expand Down Expand Up @@ -466,8 +465,7 @@ func (cfg *Config) addAutoNAT(h *bhost.BasicHost) error {
}

// Pull out the pieces of the config that we _actually_ care about.
// Specifically, don't set up things like autorelay, listeners,
// identify, etc.
// Specifically, don't set up things like listeners, identify, etc.
autoNatCfg := Config{
Transports: cfg.Transports,
Muxers: cfg.Muxers,
Expand All @@ -486,30 +484,39 @@ func (cfg *Config) addAutoNAT(h *bhost.BasicHost) error {
},
}

dialer, err := autoNatCfg.makeSwarm(eventbus.NewBus(), false)
if err != nil {
return err
}
dialerHost := blankhost.NewBlankHost(dialer)
fxopts, err := autoNatCfg.addTransports()
if err != nil {
dialerHost.Close()
return err
}
var dialer *swarm.Swarm

fxopts = append(fxopts,
fx.Supply(dialerHost.ID()),
fx.Supply(dialer),
fx.Provide(eventbus.NewBus),
fx.Provide(func(lifecycle fx.Lifecycle, b event.Bus) (*swarm.Swarm, error) {
lifecycle.Append(fx.Hook{
OnStop: func(context.Context) error {
return ps.Close()
}})
var err error
dialer, err = autoNatCfg.makeSwarm(b, false)
return dialer, err

}),
fx.Provide(func() crypto.PrivKey { return autonatPrivKey }),
)
app := fx.New(fxopts...)
if err := app.Err(); err != nil {
dialerHost.Close()
return err
}
// NOTE: We're dropping the blank host here but that's fine. It
// doesn't really _do_ anything and doesn't even need to be
// closed (as long as we close the underlying network).
autonatOpts = append(autonatOpts, autonat.EnableService(dialerHost.Network()))
err = app.Start(context.Background())
if err != nil {
return err
}
go func() {
<-dialer.Done() // The swarm used for autonat has closed, we can cleanup now
app.Stop(context.Background())
}()
autonatOpts = append(autonatOpts, autonat.EnableService(dialer))
}
if cfg.AutoNATConfig.ForceReachability != nil {
autonatOpts = append(autonatOpts, autonat.WithReachability(*cfg.AutoNATConfig.ForceReachability))
Expand Down
7 changes: 0 additions & 7 deletions leaky_tests/leaky_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"testing"

"github.com/libp2p/go-libp2p"
"github.com/stretchr/testify/require"
)

func TestBadTransportConstructor(t *testing.T) {
Expand All @@ -18,9 +17,3 @@ func TestBadTransportConstructor(t *testing.T) {
t.Error("expected error to contain debugging info")
}
}

func TestAutoNATService(t *testing.T) {
h, err := libp2p.New(libp2p.EnableNATService())
require.NoError(t, err)
h.Close()
}
6 changes: 6 additions & 0 deletions libp2p_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,12 @@ func TestRoutedHost(t *testing.T) {
require.Equal(t, []peer.ID{id}, mockRouter.queried)
}

func TestAutoNATService(t *testing.T) {
h, err := New(EnableNATService())
require.NoError(t, err)
h.Close()
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(
m,
Expand Down
5 changes: 4 additions & 1 deletion p2p/host/autonat/autonat.go
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,9 @@ func (as *AmbientAutoNAT) Close() error {
as.service.Disable()
}
<-as.backgroundRunning
if as.service != nil {
return as.service.Close()
}
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

Expand All @@ -444,7 +447,7 @@ func (s *StaticAutoNAT) Status() network.Reachability {

func (s *StaticAutoNAT) Close() error {
if s.service != nil {
s.service.Disable()
return s.service.Close()
}
return nil
}
5 changes: 5 additions & 0 deletions p2p/host/autonat/svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ func (as *autoNATService) Disable() {
}
}

func (as *autoNATService) Close() error {
as.Disable()
return as.config.dialer.Close()
}

Comment on lines +276 to +280
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just close the dialer from the autonat instance? That seems better, we ensure that the dialer is always closed even if for some reason there's an issue with service being set to nil for some reason.
I don't have strong opinions here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My read here is that the dialer is owned by the service, so it's the service's responsibility to close it.

func (as *autoNATService) background(ctx context.Context) {
defer close(as.backgroundRunning)

Expand Down
5 changes: 5 additions & 0 deletions p2p/net/swarm/swarm.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ func (s *Swarm) Close() error {
return nil
}

// Done returns a channel that is closed when the swarm is closed.
func (s *Swarm) Done() <-chan struct{} {
return s.ctx.Done()
}

func (s *Swarm) close() {
s.ctxCancel()

Expand Down
Loading