fix: get rid of shutdown errors
Instead of feeding through the top-level context, feed through a cancel-free context (that still carries the same context values). Then, when the top-level context is canceled, call `stop` to shut everything down in-order. Finally, cancel the inner context to make sure everything has been cleaned up. Ideally, we just wouldn't use contexts for this. But this is strictly better than what we have.
Steven Allen committed
Mar 29, 2020 at 20:48 UTC
efdb8db276910bc177d4c0f758170623505609c4
1 file changed
+32
-6
core/builder.go
+32
-6
@@ -3,6 +3,7 @@ package core
3
import (
4
"context"
5
"sync"
6
+ "time"
7
8
"github.com/ipfs/go-ipfs/core/bootstrap"
9
"github.com/ipfs/go-ipfs/core/node"
@@ -11,10 +12,26 @@ import (
12
"go.uber.org/fx"
13
)
14
15
+// from https://stackoverflow.com/a/59348871
16
+type valueContext struct {
17
+ context.Context
18
+}
19
+
20
+func (valueContext) Deadline() (deadline time.Time, ok bool) { return }
21
+func (valueContext) Done() <-chan struct{} { return nil }
22
+func (valueContext) Err() error { return nil }
23
+
24
type BuildCfg = node.BuildCfg // Alias for compatibility until we properly refactor the constructor interface
25
26
// NewNode constructs and returns an IpfsNode using the given cfg.
27
func NewNode(ctx context.Context, cfg *BuildCfg) (*IpfsNode, error) {
28
+ // save this context as the "lifetime" ctx.
29
+ lctx := ctx
30
+
31
+ // derive a new context that ignores cancellations from the lifetime ctx.
32
+ ctx, cancel := context.WithCancel(valueContext{ctx})
33
+
34
+ // add a metrics scope.
35
ctx = metrics.CtxScope(ctx, "ipfs")
36
37
n := &IpfsNode{
@@ -33,18 +50,27 @@ func NewNode(ctx context.Context, cfg *BuildCfg) (*IpfsNode, error) {
50
n.stop = func() error {
51
once.Do(func() {
52
stopErr = app.Stop(context.Background())
53
+ if stopErr != nil {
54
+ log.Error("failure on stop: ", stopErr)
55
+ }
56
+ // Cancel the context _after_ the app has stopped.
57
+ cancel()
58
})
59
return stopErr
60
}
61
n.IsOnline = cfg.Online
62
63
go func() {
42
- // Note that some services use contexts to signal shutting down, which is
43
- // very suboptimal. This needs to be here until that's addressed somehow
44
- <-ctx.Done()
45
- err := n.stop()
46
- if err != nil {
47
- log.Error("failure on stop: ", err)
64
+ // Shut down the application if the lifetime context is canceled.
65
+ // NOTE: we _should_ stop the application by calling `Close()`
66
+ // on the process. But we currently manage everything with contexts.
67
+ select {
68
+ case <-lctx.Done():
69
+ err := n.stop()
70
+ if err != nil {
71
+ log.Error("failure on stop: ", err)
72
+ }
73
+ case <-ctx.Done():
74
}
75
}()
76