runtimes/google/ipc: cleanup error handling.
MultiPart: 1/2
Change-Id: I366b9cecc1b53d4a2ffc207e5ed52f7e7bf9c3e4
diff --git a/runtimes/google/ipc/cancel_test.go b/runtimes/google/ipc/cancel_test.go
index d5e2e01..2022ad7 100644
--- a/runtimes/google/ipc/cancel_test.go
+++ b/runtimes/google/ipc/cancel_test.go
@@ -21,13 +21,13 @@
}
type canceld struct {
- sm stream.Manager
- ns naming.Namespace
- name string
- child string
- started chan struct{}
- cancelled chan struct{}
- stop func() error
+ sm stream.Manager
+ ns naming.Namespace
+ name string
+ child string
+ started chan struct{}
+ canceled chan struct{}
+ stop func() error
}
func (c *canceld) Run(ctx ipc.ServerCall) error {
@@ -48,8 +48,8 @@
vlog.Info(c.name, " waiting for cancellation")
<-ctx.Context().Done()
- vlog.Info(c.name, " cancelled")
- close(c.cancelled)
+ vlog.Info(c.name, " canceled")
+ close(c.canceled)
return nil
}
@@ -65,13 +65,13 @@
}
c := &canceld{
- sm: sm,
- ns: ns,
- name: name,
- child: child,
- started: make(chan struct{}, 0),
- cancelled: make(chan struct{}, 0),
- stop: s.Stop,
+ sm: sm,
+ ns: ns,
+ name: name,
+ child: child,
+ started: make(chan struct{}, 0),
+ canceled: make(chan struct{}, 0),
+ stop: s.Stop,
}
if err := s.Serve(name, c, fakeAuthorizer(0)); err != nil {
@@ -116,7 +116,7 @@
vlog.Info("cancelling initial call")
cancel()
- vlog.Info("waiting for children to be cancelled")
- <-c1.cancelled
- <-c2.cancelled
+ vlog.Info("waiting for children to be canceled")
+ <-c1.canceled
+ <-c2.canceled
}
diff --git a/runtimes/google/ipc/client.go b/runtimes/google/ipc/client.go
index 5f53bc8..a534864 100644
--- a/runtimes/google/ipc/client.go
+++ b/runtimes/google/ipc/client.go
@@ -53,7 +53,6 @@
errAuthError = verror.Register(pkgPath+".authError", verror.RetryRefetch, "authentication error from server {3}{:4}")
errSystemRetry = verror.Register(pkgPath+".sysErrorRetryConnection", verror.RetryConnection, "{:3:}")
- errClosingFlow = verror.Register(pkgPath+".errClosingFlow", verror.NoRetry, "{:3:}")
errVomEncoder = verror.Register(pkgPath+".vomEncoder", verror.NoRetry, "failed to create vom encoder {:3}")
errVomDecoder = verror.Register(pkgPath+".vomDecoder", verror.NoRetry, "failed to create vom decoder {:3}")
@@ -315,7 +314,7 @@
}
// startCall ensures StartCall always returns verror.E.
-func (c *client) startCall(ctx *context.T, name, method string, args []interface{}, opts []ipc.CallOpt) (ipc.Call, verror.E) {
+func (c *client) startCall(ctx *context.T, name, method string, args []interface{}, opts []ipc.CallOpt) (ipc.Call, error) {
if !ctx.Initialized() {
return nil, verror.ExplicitMake(verror.BadArg, i18n.NoLangID, "ipc.Client", "StartCall")
}
@@ -334,7 +333,7 @@
deadline = time.Now().Add(time.Duration(r))
}
- var lastErr verror.E
+ var lastErr error
for retries := 0; ; retries++ {
if retries != 0 {
if !backoff(retries, deadline) {
@@ -412,16 +411,19 @@
// (all that serve "name"), but will invoke the method on at most one of them
// (the server running on the most preferred protcol and network amongst all
// the servers that were successfully connected to and authorized).
-func (c *client) tryCall(ctx *context.T, name, method string, args []interface{}, opts []ipc.CallOpt) (ipc.Call, verror.ActionCode, verror.E) {
+func (c *client) tryCall(ctx *context.T, name, method string, args []interface{}, opts []ipc.CallOpt) (ipc.Call, verror.ActionCode, error) {
var resolved *naming.MountEntry
var pattern security.BlessingPattern
var err error
if resolved, err = c.ns.Resolve(ctx, name, getResolveOpts(opts)...); err != nil {
vlog.Errorf("Resolve: %v", err)
+ // We always return NoServers as the error so that the caller knows
+ // that's ok to retry the operation since the name may be registered
+ // in the near future.
if verror.Is(err, naming.ErrNoSuchName.ID) {
return nil, verror.RetryRefetch, verror.Make(verror.NoServers, ctx, name)
}
- return nil, verror.NoRetry, verror.Make(verror.NoExist, ctx, name, err)
+ return nil, verror.NoRetry, verror.Make(verror.NoServers, ctx, name, err)
} else {
pattern = security.BlessingPattern(resolved.Pattern)
if len(resolved.Servers) == 0 {
@@ -731,14 +733,33 @@
return fc, nil
}
-func (fc *flowClient) close(verr verror.E) verror.E {
- if err := fc.flow.Close(); err != nil && verr == nil {
- verr = verror.Make(errClosingFlow, fc.ctx, err)
+func (fc *flowClient) close(err error) error {
+ if cerr := fc.flow.Close(); cerr != nil && err == nil {
+ return verror.Make(verror.Internal, fc.ctx, err)
}
- return verr
+ switch {
+ case verror.Is(err, verror.BadProtocol.ID):
+ switch fc.ctx.Err() {
+ case context.DeadlineExceeded:
+ // TODO(cnicolaou,m3b): reintroduce 'append' when the new verror API is done.
+ //return verror.Append(verror.Make(verror.Timeout, fc.ctx), verr)
+ return verror.Make(verror.Timeout, fc.ctx, err.Error())
+ case context.Canceled:
+ // TODO(cnicolaou,m3b): reintroduce 'append' when the new verror API is done.
+ //return verror.Append(verror.Make(verror.Canceled, fc.ctx), verr)
+ return verror.Make(verror.Canceled, fc.ctx, err.Error())
+ }
+ case verror.Is(err, verror.Timeout.ID):
+ // Canceled trumps timeout.
+ if fc.ctx.Err() == context.Canceled {
+ // TODO(cnicolaou,m3b): reintroduce 'append' when the new verror API is done.
+ return verror.Make(verror.Canceled, fc.ctx, err.Error())
+ }
+ }
+ return err
}
-func (fc *flowClient) start(suffix, method string, args []interface{}, timeout time.Duration, blessings security.Blessings) verror.E {
+func (fc *flowClient) start(suffix, method string, args []interface{}, timeout time.Duration, blessings security.Blessings) error {
// Fetch any discharges for third-party caveats on the client's blessings
// if this client owns a discharge-client.
if self := fc.flow.LocalBlessings(); self != nil && fc.dc != nil {
@@ -800,10 +821,10 @@
func decodeNetError(ctx *context.T, err error) verror.IDAction {
if neterr, ok := err.(net.Error); ok {
if neterr.Timeout() || neterr.Temporary() {
- // If a read is cancelled in the lower levels we see
+ // If a read is canceled in the lower levels we see
// a timeout error - see readLocked in vc/reader.go
if ctx.Err() == context.Canceled {
- return verror.Cancelled
+ return verror.Canceled
}
return verror.Timeout
}
@@ -888,7 +909,7 @@
}
// finish ensures Finish always returns verror.E.
-func (fc *flowClient) finish(resultptrs ...interface{}) verror.E {
+func (fc *flowClient) finish(resultptrs ...interface{}) error {
if fc.finished {
err := verror.Make(errClientFinishAlreadyCalled, fc.ctx)
return fc.close(verror.Make(verror.BadState, fc.ctx, err))
diff --git a/runtimes/google/ipc/client_test.go b/runtimes/google/ipc/client_test.go
index ad6eca7..b91f8c0 100644
--- a/runtimes/google/ipc/client_test.go
+++ b/runtimes/google/ipc/client_test.go
@@ -205,17 +205,10 @@
return eps[0].Name(), deferFn
}
-func testForVerror(t *testing.T, err error, verr ...verror.IDAction) {
+func testForVerror(t *testing.T, err error, verr verror.IDAction) {
_, file, line, _ := runtime.Caller(1)
loc := fmt.Sprintf("%s:%d", filepath.Base(file), line)
- found := false
- for _, v := range verr {
- if verror.Is(err, v.ID) {
- found = true
- break
- }
- }
- if !found {
+ if !verror.Is(err, verr.ID) {
if _, ok := err.(verror.E); !ok {
t.Fatalf("%s: err %v not a verror", loc, err)
}
@@ -235,12 +228,11 @@
ctx, _ = context.WithTimeout(ctx, 100*time.Millisecond)
call, err := veyron2.GetClient(ctx).StartCall(ctx, name, "Sleep", nil)
if err != nil {
- testForVerror(t, err, verror.Timeout, verror.BadProtocol)
+ testForVerror(t, err, verror.Timeout)
return
}
verr := call.Finish(&err)
- // TODO(cnicolaou): this should be Timeout only.
- testForVerror(t, verr, verror.Timeout, verror.BadProtocol)
+ testForVerror(t, verr, verror.Timeout)
}
func TestArgsAndResponses(t *testing.T) {
@@ -291,7 +283,7 @@
testForVerror(t, verr, verror.NoAccess)
}
-func TestCancelledBeforeFinish(t *testing.T) {
+func TestCanceledBeforeFinish(t *testing.T) {
ctx, shutdown := newCtx()
defer shutdown()
name, fn := initServer(t, ctx)
@@ -305,11 +297,11 @@
// Cancel before we call finish.
cancel()
verr := call.Finish(&err)
- // TOO(cnicolaou): this should be Cancelled only.
- testForVerror(t, verr, verror.Cancelled, verror.BadProtocol)
+ // TOO(cnicolaou): this should be Canceled only.
+ testForVerror(t, verr, verror.Canceled)
}
-func TestCancelledDuringFinish(t *testing.T) {
+func TestCanceledDuringFinish(t *testing.T) {
ctx, shutdown := newCtx()
defer shutdown()
name, fn := initServer(t, ctx)
@@ -326,8 +318,7 @@
cancel()
}()
verr := call.Finish(&err)
- // TOO(cnicolaou): this should be Cancelled only.
- testForVerror(t, verr, verror.Cancelled, verror.BadProtocol)
+ testForVerror(t, verr, verror.Canceled)
}
func TestRendezvous(t *testing.T) {
@@ -358,7 +349,7 @@
response := ""
verr := call.Finish(&response, &err)
if verr != nil {
- testForVerror(t, verr, verror.Cancelled)
+ testForVerror(t, verr, verror.Canceled)
return
}
if got, want := response, "message: hello\n"; got != want {
@@ -395,8 +386,8 @@
ctx, _ = context.WithTimeout(ctx, 300*time.Millisecond)
call, err := veyron2.GetClient(ctx).StartCall(ctx, name, "Source", []interface{}{want})
if err != nil {
- if !verror.Is(err, verror.Timeout.ID) && !verror.Is(err, verror.BadProtocol.ID) {
- t.Fatalf("verror should be a timeout or badprotocol, not %s: stack %s",
+ if !verror.Is(err, verror.Timeout.ID) {
+ t.Fatalf("verror should be a timeout not %s: stack %s",
err, err.(verror.E).Stack())
}
return
@@ -413,11 +404,11 @@
continue
}
// TOO(cnicolaou): this should be Timeout only.
- testForVerror(t, err, verror.Timeout, verror.BadProtocol)
+ testForVerror(t, err, verror.Timeout)
break
}
verr := call.Finish(&err)
- testForVerror(t, verr, verror.Timeout, verror.BadProtocol)
+ testForVerror(t, verr, verror.Timeout)
}
func TestStreamAbort(t *testing.T) {
@@ -465,14 +456,14 @@
_, fn := runMountTable(t, ctx)
defer fn()
name := "noservers"
- ctx, _ = context.WithTimeout(ctx, 300*time.Millisecond)
+ ctx, _ = context.WithTimeout(ctx, 1000*time.Millisecond)
call, verr := veyron2.GetClient(ctx).StartCall(ctx, name, "Sleep", nil)
if verr != nil {
- testForVerror(t, verr, verror.Timeout, verror.BadProtocol, verror.NoExist)
+ testForVerror(t, verr, verror.NoServers)
return
}
err := call.Finish(&verr)
- testForVerror(t, err, verror.Timeout, verror.BadProtocol, verror.NoExist)
+ testForVerror(t, err, verror.NoServers)
}
func TestNoMountTable(t *testing.T) {
diff --git a/services/mgmt/logreader/impl/reader.go b/services/mgmt/logreader/impl/reader.go
index e2cefb9..2d1908a 100644
--- a/services/mgmt/logreader/impl/reader.go
+++ b/services/mgmt/logreader/impl/reader.go
@@ -47,7 +47,7 @@
if f.ctx != nil {
select {
case <-f.ctx.Context().Done():
- return 0, verror.Make(verror.Cancelled, nil)
+ return 0, verror.Make(verror.Canceled, nil)
default:
}
}
