v.io: fix places where context.T cancellation functions are not called.
context.T (like its standard Go namesake) requires that the cancellation
function returned by WithCancel() be called, of the context object will be
leaked. The same is true of WithTimeout() to a lesser degree---one might
ignore the issue if the timeout is short.
This change fixed a number of places where we curently fail to call the
cancellation function.
Some are only on error paths that will be taken only rarely.
Some are on timeout cases, which we could ignore, but for the moment, I'm
trying to be thorough, primarily because existing code acts as a template for
future code, and it seems better to encourage good habits.
In a couple of cases, the existing code is correct, but I added a comment to
explain that a call is not needed.
MultiPart: 2/2
Change-Id: Ia76a70dbc56d43d051862f406e5193871f29af4a
diff --git a/lib/discovery/advertise.go b/lib/discovery/advertise.go
index 8b20b50..67d43cb 100644
--- a/lib/discovery/advertise.go
+++ b/lib/discovery/advertise.go
@@ -124,6 +124,7 @@
for _, plugin := range d.plugins {
wg.Add(1)
if err := plugin.Advertise(ctx, adinfo, wg.Done); err != nil {
+ cancel()
return nil, err
}
}
diff --git a/lib/discovery/plugins/vine/store.go b/lib/discovery/plugins/vine/store.go
index 16d53a1..4894e4f 100644
--- a/lib/discovery/plugins/vine/store.go
+++ b/lib/discovery/plugins/vine/store.go
@@ -152,6 +152,7 @@
// TODO(suharshs): Make the authorizer configurable?
_, server, err := v23.WithNewServer(ctx, name, StoreServer(s), security.AllowEveryone())
if err != nil {
+ cancel()
return nil, err
}
go s.flushExpired(ctx)
diff --git a/lib/raft/raft.go b/lib/raft/raft.go
index 6ea5c03..264615f 100644
--- a/lib/raft/raft.go
+++ b/lib/raft/raft.go
@@ -366,7 +366,8 @@
func (r *raft) startElection() {
// If we can't get a response in 2 seconds, something is really wrong.
- ctx, _ := context.WithTimeout(r.ctx, 2*time.Second)
+ ctx, cancel := context.WithTimeout(r.ctx, 2*time.Second)
+ defer cancel()
if err := r.p.IncCurrentTerm(); err != nil {
// If this fails, there's no way to recover.
vlog.Fatalf("incrementing current term: %s", err)
@@ -627,9 +628,10 @@
// Send to the follower. We drop the lock while we do this. That means we may stop being the
// leader in the middle of the call but that's OK as long as we check when we get it back.
r.Unlock()
- ctx, _ := context.WithTimeout(r.ctx, time.Duration(2)*time.Second)
+ ctx, cancel := context.WithTimeout(r.ctx, time.Duration(2)*time.Second)
client := v23.GetClient(ctx)
err := client.Call(ctx, m.id, "AppendToLog", msg, []interface{}{}, options.Preresolved{})
+ cancel()
r.Lock()
if r.role != RoleLeader {
// Not leader any more, doesn't matter how he replied.
@@ -677,7 +679,8 @@
if err != nil {
return 0, err
}
- ctx, _ := context.WithTimeout(r.ctx, time.Duration(5*60)*time.Second)
+ ctx, cancel := context.WithTimeout(r.ctx, time.Duration(5*60)*time.Second)
+ defer cancel()
client := raftProtoClient(m.id)
call, err := client.InstallSnapshot(ctx, r.p.CurrentTerm(), r.me.id, term, index, options.Preresolved{})
if err != nil {
diff --git a/runtime/internal/naming/namespace/glob.go b/runtime/internal/naming/namespace/glob.go
index 5106b02..f582358 100644
--- a/runtime/internal/naming/namespace/glob.go
+++ b/runtime/internal/naming/namespace/glob.go
@@ -79,7 +79,9 @@
// but it avoids making yet another copy of the MountEntry.
on := t.me.Name
t.me.Name = ""
- call, err := client.StartCall(withTimeout(ctx), "", rpc.GlobMethod, []interface{}{pstr}, append(opts, options.Preresolved{t.me})...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ call, err := client.StartCall(timeoutCtx, "", rpc.GlobMethod, []interface{}{pstr}, append(opts, options.Preresolved{t.me})...)
t.me.Name = on
if err != nil {
t.error = err
diff --git a/runtime/internal/naming/namespace/mount.go b/runtime/internal/naming/namespace/mount.go
index 9c0390c..71b4fb3 100644
--- a/runtime/internal/naming/namespace/mount.go
+++ b/runtime/internal/naming/namespace/mount.go
@@ -47,7 +47,9 @@
me, err := ns.ResolveToMountTable(ctx, name, opts...)
if err == nil {
copts := append(getCallOpts(opts), options.Preresolved{me})
- err = v23.GetClient(ctx).Call(withTimeout(ctx), name, "Mount", []interface{}{server, uint32(ttl.Seconds()), flags}, nil, copts...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ err = v23.GetClient(ctx).Call(timeoutCtx, name, "Mount", []interface{}{server, uint32(ttl.Seconds()), flags}, nil, copts...)
ns.forget(ctx, me)
}
ctx.VI(1).Infof("Mount(%s, %q) -> %v", name, server, err)
@@ -60,7 +62,9 @@
me, err := ns.ResolveToMountTable(ctx, name, opts...)
if err == nil {
copts := append(getCallOpts(opts), options.Preresolved{me})
- err = v23.GetClient(ctx).Call(withTimeout(ctx), name, "Unmount", []interface{}{server}, nil, copts...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ err = v23.GetClient(ctx).Call(timeoutCtx, name, "Unmount", []interface{}{server}, nil, copts...)
ns.forget(ctx, me)
}
ctx.VI(1).Infof("Unmount(%s, %s) -> %v", name, server, err)
@@ -73,7 +77,9 @@
me, err := ns.ResolveToMountTable(ctx, name, opts...)
if err == nil {
copts := append(getCallOpts(opts), options.Preresolved{me})
- err = v23.GetClient(ctx).Call(withTimeout(ctx), name, "Delete", []interface{}{deleteSubtree}, nil, copts...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ err = v23.GetClient(ctx).Call(timeoutCtx, name, "Delete", []interface{}{deleteSubtree}, nil, copts...)
ns.forget(ctx, me)
}
ctx.VI(1).Infof("Remove(%s, %v) -> %v", name, deleteSubtree, err)
diff --git a/runtime/internal/naming/namespace/namespace.go b/runtime/internal/naming/namespace/namespace.go
index 8ab331c..b6e1198 100644
--- a/runtime/internal/naming/namespace/namespace.go
+++ b/runtime/internal/naming/namespace/namespace.go
@@ -180,12 +180,17 @@
// All operations against the mount table service use this fixed timeout unless overridden.
const callTimeout = 30 * time.Second
+func emptyFunc() {}
+
// withTimeout returns a new context if the orinal has no timeout set.
-func withTimeout(ctx *context.T) *context.T {
+func withTimeout(ctx *context.T) (*context.T, func()) {
+ var cancel func()
if _, ok := ctx.Deadline(); !ok {
- ctx, _ = context.WithTimeout(ctx, callTimeout)
+ ctx, cancel = context.WithTimeout(ctx, callTimeout)
+ } else {
+ cancel = emptyFunc
}
- return ctx
+ return ctx, cancel
}
// CacheCtl implements namespace.T.CacheCtl
diff --git a/runtime/internal/naming/namespace/perms.go b/runtime/internal/naming/namespace/perms.go
index aa3ce6c..bab4882 100644
--- a/runtime/internal/naming/namespace/perms.go
+++ b/runtime/internal/naming/namespace/perms.go
@@ -20,7 +20,9 @@
me, err := ns.ResolveToMountTable(ctx, name, opts...)
if err == nil {
copts := append(getCallOpts(opts), options.Preresolved{me})
- err = v23.GetClient(ctx).Call(withTimeout(ctx), name, "SetPermissions", []interface{}{perms, version}, nil, copts...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ err = v23.GetClient(ctx).Call(timeoutCtx, name, "SetPermissions", []interface{}{perms, version}, nil, copts...)
ns.forget(ctx, me)
}
ctx.VI(1).Infof("SetPermissions(%s, %v, %s) -> %v", name, perms, version, err)
@@ -34,7 +36,9 @@
me, err := ns.ResolveToMountTable(ctx, name, opts...)
if err == nil {
copts := append(getCallOpts(opts), options.Preresolved{me})
- err = v23.GetClient(ctx).Call(withTimeout(ctx), name, "GetPermissions", []interface{}{}, []interface{}{&perms, &version}, copts...)
+ timeoutCtx, cancel := withTimeout(ctx)
+ defer cancel()
+ err = v23.GetClient(ctx).Call(timeoutCtx, name, "GetPermissions", []interface{}{}, []interface{}{&perms, &version}, copts...)
ns.forget(ctx, me)
}
ctx.VI(1).Infof("GetPermissions(%s) -> (%v, %v, %v)", name, perms, version, err)
diff --git a/runtime/internal/naming/namespace/resolve.go b/runtime/internal/naming/namespace/resolve.go
index 2b3438d..16859d2 100644
--- a/runtime/internal/naming/namespace/resolve.go
+++ b/runtime/internal/naming/namespace/resolve.go
@@ -65,7 +65,9 @@
if _, hasDeadline := ctx.Deadline(); !hasDeadline {
// Only set a per-call timeout if a deadline has not already
// been set.
- callCtx = withTimeout(ctx)
+ var cancel func()
+ callCtx, cancel = withTimeout(ctx)
+ defer cancel()
}
entry := new(naming.MountEntry)
if err := client.Call(callCtx, e.Name, "ResolveStep", nil, []interface{}{entry}, opts...); err != nil {
diff --git a/runtime/internal/rt/mgmt.go b/runtime/internal/rt/mgmt.go
index e62c039..6d2131c 100644
--- a/runtime/internal/rt/mgmt.go
+++ b/runtime/internal/rt/mgmt.go
@@ -90,6 +90,7 @@
ctx, cancel := context.WithCancel(ctx)
_, server, err := rt.WithNewServer(ctx, "", v23.GetAppCycle(ctx).Remote(), nil)
if err != nil {
+ cancel()
return err
}
if err = rt.callbackToParent(ctx, parentName, server.Status().Endpoints[0].Name()); err != nil {
@@ -97,6 +98,7 @@
<-server.Closed()
return err
}
+ // cancel() is never called if we return without error.
return nil
}
@@ -120,6 +122,7 @@
}
func (rt *Runtime) callbackToParent(ctx *context.T, parentName, myName string) error {
- ctx, _ = context.WithTimeout(ctx, time.Minute)
- return rt.GetClient(ctx).Call(ctx, parentName, "Set", []interface{}{mgmt.AppCycleManagerConfigKey, myName}, nil)
+ timeoutCtx, cancel := context.WithTimeout(ctx, time.Minute)
+ defer cancel()
+ return rt.GetClient(timeoutCtx).Call(timeoutCtx, parentName, "Set", []interface{}{mgmt.AppCycleManagerConfigKey, myName}, nil)
}
diff --git a/services/debug/debug/browseserver/browseserver.go b/services/debug/debug/browseserver/browseserver.go
index a59d12a..18761a6 100644
--- a/services/debug/debug/browseserver/browseserver.go
+++ b/services/debug/debug/browseserver/browseserver.go
@@ -220,9 +220,8 @@
}
}
-func (h *handler) withTimeout(ctx *context.T) *context.T {
- ctx, _ = context.WithTimeout(ctx, h.timeout)
- return ctx
+func (h *handler) withTimeout(ctx *context.T) (*context.T, func()) {
+ return context.WithTimeout(ctx, h.timeout)
}
type resolveHandler struct{ *handler }
@@ -231,7 +230,9 @@
name := r.FormValue("n")
var suffix string
ctx, tracer := newTracer(h.ctx)
- m, err := v23.GetNamespace(ctx).Resolve(h.withTimeout(ctx), name)
+ timeoutCtx, cancel := h.withTimeout(ctx)
+ defer cancel()
+ m, err := v23.GetNamespace(ctx).Resolve(timeoutCtx, name)
if m != nil {
suffix = m.Name
}
@@ -256,7 +257,8 @@
func (h *blessingsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
name := r.FormValue("n")
ctx, tracer := newTracer(h.ctx)
- call, err := v23.GetClient(ctx).StartCall(h.withTimeout(ctx), name, "DoNotReallyCareAboutTheMethod", nil)
+ timeoutCtx, cancel := h.withTimeout(ctx)
+ call, err := v23.GetClient(ctx).StartCall(timeoutCtx, name, "DoNotReallyCareAboutTheMethod", nil)
args := struct {
ServerName string
CommandLine string
@@ -275,7 +277,10 @@
args.Recognized, args.Blessings = call.RemoteBlessings()
args.CertificateChains = security.MarshalBlessings(args.Blessings).CertificateChains
// Don't actually care about the RPC, so don't bother waiting on the Finish.
+ cancel()
defer func() { go call.Finish() }()
+ } else {
+ cancel()
}
h.execute(h.ctx, w, r, "blessings.html", args)
}
@@ -291,7 +296,9 @@
ctx, tracer = newTracer(h.ctx)
hasValue = true
)
- v, err := stats.StatsClient(name).Value(h.withTimeout(ctx))
+ timeoutCtx, cancel := h.withTimeout(ctx)
+ defer cancel()
+ v, err := stats.StatsClient(name).Value(timeoutCtx)
if verror.ErrorID(err) == verror.ErrNoExist.ID {
// Stat does not exist as a value, that's okay.
err = nil
@@ -299,7 +306,9 @@
}
var children []string
var childrenErrors []error
- if glob, globErr := v23.GetNamespace(ctx).Glob(h.withTimeout(ctx), naming.Join(name, "*")); globErr == nil {
+ timeoutCtx2, cancel2 := h.withTimeout(ctx)
+ defer cancel2()
+ if glob, globErr := v23.GetNamespace(ctx).Glob(timeoutCtx2, naming.Join(name, "*")); globErr == nil {
for e := range glob {
switch e := e.(type) {
case *naming.GlobReplyEntry:
@@ -484,7 +493,9 @@
ctx, tracer = newTracer(h.ctx)
entries []entry
)
- ch, err := v23.GetNamespace(ctx).Glob(h.withTimeout(ctx), pattern)
+ timeoutCtx, cancel := h.withTimeout(ctx)
+ defer cancel()
+ ch, err := v23.GetNamespace(ctx).Glob(timeoutCtx, pattern)
if err != nil {
entries = append(entries, entry{Error: err})
}
diff --git a/services/device/deviced/internal/starter/starter.go b/services/device/deviced/internal/starter/starter.go
index 82f7980..e269ff0 100644
--- a/services/device/deviced/internal/starter/starter.go
+++ b/services/device/deviced/internal/starter/starter.go
@@ -264,6 +264,7 @@
ctx, cancel := context.WithCancel(ctx)
ctx, server, err := v23.WithNewDispatchingServer(ctx, args.name(mt), wrapper)
if err != nil {
+ cancel()
return nil, err
}
args.ConfigState.Name = server.Status().Endpoints[0].Name()
diff --git a/services/syncbase/ping/ping.go b/services/syncbase/ping/ping.go
index 123bcbe..0d5daab 100644
--- a/services/syncbase/ping/ping.go
+++ b/services/syncbase/ping/ping.go
@@ -33,12 +33,13 @@
// changes during the call. Clients must be resilient to such errors.
func PingInParallel(ctx *context.T, names []string, timeout, channelTimeout time.Duration) (map[string]PingResult, error) {
results := map[string]PingResult{}
- ctx, _ = context.WithTimeout(ctx, timeout)
+ timeoutCtx, cancel := context.WithTimeout(ctx, timeout)
+ defer cancel()
resultChan := make(chan PingResult)
// Spawn goroutines.
for _, name := range names {
- // Note, ctx is thread-safe and is not mutated by the spawned goroutines.
- go ping(ctx, name, channelTimeout, resultChan)
+ // Note, timeoutCtx is thread-safe and is not mutated by the spawned goroutines.
+ go ping(timeoutCtx, name, channelTimeout, resultChan)
}
for range names {
r := <-resultChan
diff --git a/services/syncbase/vsync/initiator.go b/services/syncbase/vsync/initiator.go
index 1e42c4d..36d0347 100644
--- a/services/syncbase/vsync/initiator.go
+++ b/services/syncbase/vsync/initiator.go
@@ -1269,6 +1269,7 @@
if err == nil {
vlog.VI(4).Infof("sync: runRemoteOp: end op established for %s", absName)
+ // Responsibility for calling cancel() is passed to caller.
return resp, nil
}
