Skip to content

Commit 8cc74d3

Browse files
marten-seemannmarten-seemann
authored
Merge pull request #109 from libp2p/clean-shutdown
remove context from constructor, implement a proper Close method
2 parents 0eefbaa + 8961023 commit 8cc74d3

File tree

9 files changed

+63
-1097
lines changed

9 files changed

+63
-1097
lines changed

‎p2p/host/autonat/autonat.go

+31-10
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@ var log = logging.Logger("autonat")
2222

2323
// AmbientAutoNAT is the implementation of ambient NAT autodiscovery
2424
type AmbientAutoNAT struct {
25-
ctx context.Context
2625
host host.Host
2726

2827
*config
2928

29+
ctx context.Context
30+
ctxCancel context.CancelFunc // is closed when Close is called
31+
backgroundRunning chan struct{} // is closed when the background go routine exits
32+
3033
inboundConn chan network.Conn
3134
observations chan autoNATResult
3235
// status is an autoNATResult reflecting current status.
@@ -50,7 +53,6 @@ type AmbientAutoNAT struct {
5053

5154
// StaticAutoNAT is a simple AutoNAT implementation when a single NAT status is desired.
5255
type StaticAutoNAT struct {
53-
ctx context.Context
5456
host host.Host
5557
reachability network.Reachability
5658
service *autoNATService
@@ -62,7 +64,7 @@ type autoNATResult struct {
6264
}
6365

6466
// New creates a new NAT autodiscovery system attached to a host
65-
func New(ctx context.Context, h host.Host, options ...Option) (AutoNAT, error) {
67+
func New(h host.Host, options ...Option) (AutoNAT, error) {
6668
var err error
6769
conf := new(config)
6870
conf.host = h
@@ -84,7 +86,7 @@ func New(ctx context.Context, h host.Host, options ...Option) (AutoNAT, error) {
8486

8587
var service *autoNATService
8688
if (!conf.forceReachability || conf.reachability == network.ReachabilityPublic) && conf.dialer != nil {
87-
service, err = newAutoNATService(ctx, conf)
89+
service, err = newAutoNATService(conf)
8890
if err != nil {
8991
return nil, err
9092
}
@@ -95,19 +97,21 @@ func New(ctx context.Context, h host.Host, options ...Option) (AutoNAT, error) {
9597
emitReachabilityChanged.Emit(event.EvtLocalReachabilityChanged{Reachability: conf.reachability})
9698

9799
return &StaticAutoNAT{
98-
ctx: ctx,
99100
host: h,
100101
reachability: conf.reachability,
101102
service: service,
102103
}, nil
103104
}
104105

106+
ctx, cancel := context.WithCancel(context.Background())
105107
as := &AmbientAutoNAT{
106-
ctx: ctx,
107-
host: h,
108-
config: conf,
109-
inboundConn: make(chan network.Conn, 5),
110-
observations: make(chan autoNATResult, 1),
108+
ctx: ctx,
109+
ctxCancel: cancel,
110+
backgroundRunning: make(chan struct{}),
111+
host: h,
112+
config: conf,
113+
inboundConn: make(chan network.Conn, 5),
114+
observations: make(chan autoNATResult, 1),
111115

112116
emitReachabilityChanged: emitReachabilityChanged,
113117
service: service,
@@ -159,6 +163,7 @@ func ipInList(candidate ma.Multiaddr, list []ma.Multiaddr) bool {
159163
}
160164

161165
func (as *AmbientAutoNAT) background() {
166+
defer close(as.backgroundRunning)
162167
// wait a bit for the node to come online and establish some connections
163168
// before starting autodetection
164169
delay := as.config.bootDelay
@@ -426,6 +431,15 @@ func (as *AmbientAutoNAT) getPeerToProbe() peer.ID {
426431
return candidates[0]
427432
}
428433

434+
func (as *AmbientAutoNAT) Close() error {
435+
as.ctxCancel()
436+
if as.service != nil {
437+
as.service.Disable()
438+
}
439+
<-as.backgroundRunning
440+
return nil
441+
}
442+
429443
func shufflePeers(peers []peer.ID) {
430444
for i := range peers {
431445
j := rand.Intn(i + 1)
@@ -445,3 +459,10 @@ func (s *StaticAutoNAT) PublicAddr() (ma.Multiaddr, error) {
445459
}
446460
return nil, errors.New("no available address")
447461
}
462+
463+
func (s *StaticAutoNAT) Close() error {
464+
if s.service != nil {
465+
s.service.Disable()
466+
}
467+
return nil
468+
}

‎p2p/host/autonat/autonat_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func makeAutoNAT(ctx context.Context, t *testing.T, ash host.Host) (host.Host, A
5555
h := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
5656
h.Peerstore().AddAddrs(ash.ID(), ash.Addrs(), time.Minute)
5757
h.Peerstore().AddProtocols(ash.ID(), AutoNATProto)
58-
a, _ := New(ctx, h, WithSchedule(100*time.Millisecond, time.Second), WithoutStartupDelay())
58+
a, _ := New(h, WithSchedule(100*time.Millisecond, time.Second), WithoutStartupDelay())
5959
a.(*AmbientAutoNAT).config.dialPolicy.allowSelfDials = true
6060
a.(*AmbientAutoNAT).config.throttlePeerPeriod = 100 * time.Millisecond
6161
return h, a
@@ -279,7 +279,7 @@ func TestStaticNat(t *testing.T) {
279279
h := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
280280
s, _ := h.EventBus().Subscribe(&event.EvtLocalReachabilityChanged{})
281281

282-
nat, err := New(ctx, h, WithReachability(network.ReachabilityPrivate))
282+
nat, err := New(h, WithReachability(network.ReachabilityPrivate))
283283
if err != nil {
284284
t.Fatal(err)
285285
}

‎p2p/host/autonat/interface.go

+2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package autonat
22

33
import (
44
"context"
5+
"io"
56

67
"github.com/libp2p/go-libp2p-core/network"
78
"github.com/libp2p/go-libp2p-core/peer"
@@ -16,6 +17,7 @@ type AutoNAT interface {
1617
// PublicAddr returns the public dial address when NAT status is public and an
1718
// error otherwise
1819
PublicAddr() (ma.Multiaddr, error)
20+
io.Closer
1921
}
2022

2123
// Client is a stateless client interface to AutoNAT peers

‎p2p/host/autonat/svc.go

+13-14
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ var streamTimeout = 60 * time.Second
2020

2121
// AutoNATService provides NAT autodetection services to other peers
2222
type autoNATService struct {
23-
ctx context.Context
24-
instance context.CancelFunc
25-
instanceLock sync.Mutex
23+
instanceLock sync.Mutex
24+
instance context.CancelFunc
25+
backgroundRunning chan struct{} // closed when background exits
2626

2727
config *config
2828

@@ -33,18 +33,14 @@ type autoNATService struct {
3333
}
3434

3535
// NewAutoNATService creates a new AutoNATService instance attached to a host
36-
func newAutoNATService(ctx context.Context, c *config) (*autoNATService, error) {
36+
func newAutoNATService(c *config) (*autoNATService, error) {
3737
if c.dialer == nil {
3838
return nil, errors.New("cannot create NAT service without a network")
3939
}
40-
41-
as := &autoNATService{
42-
ctx: ctx,
40+
return &autoNATService{
4341
config: c,
4442
reqs: make(map[peer.ID]int),
45-
}
46-
47-
return as, nil
43+
}, nil
4844
}
4945

5046
func (as *autoNATService) handleStream(s network.Stream) {
@@ -190,7 +186,7 @@ func (as *autoNATService) doDial(pi peer.AddrInfo) *pb.Message_DialResponse {
190186
as.globalReqs++
191187
as.mx.Unlock()
192188

193-
ctx, cancel := context.WithTimeout(as.ctx, as.config.dialTimeout)
189+
ctx, cancel := context.WithTimeout(context.Background(), as.config.dialTimeout)
194190
defer cancel()
195191

196192
as.config.dialer.Peerstore().ClearAddrs(pi.ID)
@@ -217,10 +213,11 @@ func (as *autoNATService) Enable() {
217213
if as.instance != nil {
218214
return
219215
}
220-
inst, cncl := context.WithCancel(as.ctx)
221-
as.instance = cncl
216+
ctx, cancel := context.WithCancel(context.Background())
217+
as.instance = cancel
218+
as.backgroundRunning = make(chan struct{})
222219

223-
go as.background(inst)
220+
go as.background(ctx)
224221
}
225222

226223
// Disable the autoNAT service if it is running.
@@ -230,10 +227,12 @@ func (as *autoNATService) Disable() {
230227
if as.instance != nil {
231228
as.instance()
232229
as.instance = nil
230+
<-as.backgroundRunning
233231
}
234232
}
235233

236234
func (as *autoNATService) background(ctx context.Context) {
235+
defer close(as.backgroundRunning)
237236
as.config.host.SetStreamHandler(AutoNATProto, as.handleStream)
238237

239238
timer := time.NewTimer(as.config.throttleResetPeriod)

‎p2p/host/autonat/svc_test.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ func makeAutoNATConfig(ctx context.Context, t *testing.T) *config {
2424
return &c
2525
}
2626

27-
func makeAutoNATService(ctx context.Context, t *testing.T, c *config) *autoNATService {
28-
as, err := newAutoNATService(ctx, c)
27+
func makeAutoNATService(t *testing.T, c *config) *autoNATService {
28+
as, err := newAutoNATService(c)
2929
if err != nil {
3030
t.Fatal(err)
3131
}
@@ -48,7 +48,7 @@ func TestAutoNATServiceDialError(t *testing.T) {
4848
c := makeAutoNATConfig(ctx, t)
4949
c.dialTimeout = 1 * time.Second
5050
c.dialPolicy.allowSelfDials = false
51-
_ = makeAutoNATService(ctx, t, c)
51+
_ = makeAutoNATService(t, c)
5252
hc, ac := makeAutoNATClient(ctx, t)
5353
connect(t, c.host, hc)
5454

@@ -67,7 +67,7 @@ func TestAutoNATServiceDialSuccess(t *testing.T) {
6767
defer cancel()
6868

6969
c := makeAutoNATConfig(ctx, t)
70-
_ = makeAutoNATService(ctx, t, c)
70+
_ = makeAutoNATService(t, c)
7171

7272
hc, ac := makeAutoNATClient(ctx, t)
7373
connect(t, c.host, hc)
@@ -87,7 +87,7 @@ func TestAutoNATServiceDialRateLimiter(t *testing.T) {
8787
c.throttleResetPeriod = time.Second
8888
c.throttleResetJitter = 0
8989
c.throttlePeerMax = 1
90-
_ = makeAutoNATService(ctx, t, c)
90+
_ = makeAutoNATService(t, c)
9191

9292
hc, ac := makeAutoNATClient(ctx, t)
9393
connect(t, c.host, hc)
@@ -124,7 +124,7 @@ func TestAutoNATServiceGlobalLimiter(t *testing.T) {
124124
c.throttleResetJitter = 0
125125
c.throttlePeerMax = 1
126126
c.throttleGlobalMax = 5
127-
_ = makeAutoNATService(ctx, t, c)
127+
_ = makeAutoNATService(t, c)
128128

129129
hs := c.host
130130

@@ -157,7 +157,7 @@ func TestAutoNATServiceRateLimitJitter(t *testing.T) {
157157
c.throttleResetPeriod = 100 * time.Millisecond
158158
c.throttleResetJitter = 100 * time.Millisecond
159159
c.throttleGlobalMax = 1
160-
svc := makeAutoNATService(ctx, t, c)
160+
svc := makeAutoNATService(t, c)
161161
svc.mx.Lock()
162162
svc.globalReqs = 1
163163
svc.mx.Unlock()
@@ -178,7 +178,7 @@ func TestAutoNATServiceStartup(t *testing.T) {
178178

179179
h := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
180180
dh := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
181-
an, err := New(ctx, h, EnableService(dh.Network()))
181+
an, err := New(h, EnableService(dh.Network()))
182182
an.(*AmbientAutoNAT).config.dialPolicy.allowSelfDials = true
183183
if err != nil {
184184
t.Fatal(err)

‎p2p/host/autonat/test/autonat_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
//go:build ignore
2+
// +build ignore
3+
14
// This separate testing package helps to resolve a circular dependency potentially
25
// being created between libp2p and libp2p-autonat
36
package autonat_test
@@ -31,7 +34,7 @@ func TestAutonatRoundtrip(t *testing.T) {
3134
if err != nil {
3235
t.Fatal(err)
3336
}
34-
if _, err := autonat.New(ctx, service, autonat.EnableService(dialback.Network())); err != nil {
37+
if _, err := autonat.New(service, autonat.EnableService(dialback.Network())); err != nil {
3538
t.Fatal(err)
3639
}
3740

‎p2p/host/autonat/test/dummy.go

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
package autonat_test
2+
3+
// needed so that go test ./... doesn't error

‎p2p/host/autonat/test/go.mod

-6
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,4 @@ module github.com/libp2p/go-libp2p-autonat/test
22

33
go 1.16
44

5-
require (
6-
github.com/libp2p/go-libp2p v0.14.4
7-
github.com/libp2p/go-libp2p-autonat v0.4.2
8-
github.com/libp2p/go-libp2p-core v0.8.6
9-
)
10-
115
replace github.com/libp2p/go-libp2p-autonat => ../

0 commit comments

Comments
 (0)