Merge lp:~rogpeppe/juju-core/377-fix-server-error into lp:~go-bot/juju-core/trunk

Proposed by Roger Peppe
Status: Merged
Approved by: Roger Peppe
Approved revision: no longer in the source branch.
Merged at revision: 1727
Proposed branch: lp:~rogpeppe/juju-core/377-fix-server-error
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 66 lines (+23/-3)
2 files modified
state/apiserver/common/errors.go (+13/-2)
state/apiserver/common/errors_test.go (+10/-1)
To merge this branch: bzr merge lp:~rogpeppe/juju-core/377-fix-server-error
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+182562@code.launchpad.net

Commit message

state/apiserver: make ServerError work properly

It didn't work if the error type was non-comparable.

https://codereview.appspot.com/13336043/

Description of the change

state/apiserver: make ServerError work properly

It didn't work if the error type was non-comparable.

https://codereview.appspot.com/13336043/

To post a comment you must log in.
Revision history for this message
Roger Peppe (rogpeppe) wrote :

Reviewers: mp+182562_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver: make ServerError work properly

It didn't work if the error type was non-comparable.

https://code.launchpad.net/~rogpeppe/juju-core/377-fix-server-error/+merge/182562

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13336043/

Affected files:
   A [revision details]
   M state/apiserver/common/errors.go
   M state/apiserver/common/errors_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130827122923-qx3dpiw3118bbub2
+New revision: <email address hidden>

Index: state/apiserver/common/errors.go
=== modified file 'state/apiserver/common/errors.go'
--- state/apiserver/common/errors.go 2013-08-06 17:09:11 +0000
+++ state/apiserver/common/errors.go 2013-08-28 08:45:59 +0000
@@ -51,14 +51,24 @@
   ErrNotProvisioned: params.CodeNotProvisioned,
  }

+func singletonCode(err error) string {
+ // All error types may not be hashable; deal with
+ // that by catching the panic if we try to look up
+ // a non-hashable type.
+ defer func() {
+ recover()
+ }()
+ return singletonErrorCodes[err]
+}
+
  // ServerError returns an error suitable for returning to an API
  // client, with an error code suitable for various kinds of errors
  // generated in packages outside the API.
-func ServerError(err error) *params.Error {
+func ServerError(err error) (apiError *params.Error) {
   if err == nil {
    return nil
   }
- code := singletonErrorCodes[err]
+ code := singletonCode(err)
   switch {
   case code != "":
   case errors.IsUnauthorizedError(err):

Index: state/apiserver/common/errors_test.go
=== renamed file 'state/apiserver/errors_test.go'
=> 'state/apiserver/common/errors_test.go'
--- state/apiserver/errors_test.go 2013-08-19 11:20:02 +0000
+++ state/apiserver/common/errors_test.go 2013-08-28 08:47:28 +0000
@@ -1,7 +1,7 @@
  // Copyright 2012, 2013 Canonical Ltd.
  // Licensed under the AGPLv3, see LICENCE file for details.

-package apiserver_test
+package common_test

  import (
   stderrors "errors"
@@ -71,10 +71,19 @@
   err: stderrors.New("an error"),
   code: "",
  }, {
+ err: unhashableError{"foo"},
+ code: "",
+}, {
   err: nil,
   code: "",
  }}

+type unhashableError []string
+
+func (err unhashableError) Error() string {
+ return err[0]
+}
+
  func (s *errorsSuite) TestErrorTransform(c *gc.C) {
   for _, t := range errorTransformTests {
    err1 := common.ServerError(t.err)

Revision history for this message
Roger Peppe (rogpeppe) wrote :
Revision history for this message
Roger Peppe (rogpeppe) wrote :
Revision history for this message
John A Meinel (jameinel) wrote :

Thanks for moving the tests.

I have:
   https://codereview.appspot.com/13338043

For the other way of fixing this.

I don't really care who wins.

LGTM

https://codereview.appspot.com/13336043/diff/8001/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/13336043/diff/8001/state/apiserver/common/errors.go#newcode68
state/apiserver/common/errors.go:68: func ServerError(err error)
(apiError *params.Error) {
^ I don't think this change adds anything since you are calling to a
different function to do the map lookup.

https://codereview.appspot.com/13336043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

https://codereview.appspot.com/13336043/diff/8001/state/apiserver/common/errors.go
File state/apiserver/common/errors.go (right):

https://codereview.appspot.com/13336043/diff/8001/state/apiserver/common/errors.go#newcode68
state/apiserver/common/errors.go:68: func ServerError(err error)
(apiError *params.Error) {
On 2013/08/28 09:02:40, jameinel wrote:
> ^ I don't think this change adds anything since you are calling to a
different
> function to do the map lookup.

ha, that was a relic of when i had the recovery in this function.
removed.

https://codereview.appspot.com/13336043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (44.5 KiB)

The attempt to merge lp:~rogpeppe/juju-core/377-fix-server-error into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.693s
ok launchpad.net/juju-core/agent/tools 0.215s
ok launchpad.net/juju-core/bzr 6.429s
ok launchpad.net/juju-core/cert 2.239s
ok launchpad.net/juju-core/charm 0.539s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.026s
ok launchpad.net/juju-core/cmd 0.212s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 115.176s
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x62865d]

goroutine 743 [running]:
sync/atomic.CompareAndSwapUint32()
 /usr/lib/go/src/pkg/sync/atomic/asm_amd64.s:14 +0xd
sync.(*Mutex).Lock(0x0)
 /usr/lib/go/src/pkg/sync/mutex.go:43 +0x35
launchpad.net/tomb.(*Tomb).init(0x0)
 /home/tarmac/trees/src/src/launchpad.net/tomb/tomb.go:83 +0x25
launchpad.net/tomb.(*Tomb).Kill(0x0, 0x0, 0x0)
 /home/tarmac/trees/src/src/launchpad.net/tomb/tomb.go:135 +0x25
launchpad.net/juju-core/state/api/watcher.(*commonWatcher).Stop(0x0, 0xc2002d1800, 0x0)
 /home/tarmac/trees/src/launchpad.net/juju-core/state/api/watcher/watcher.go:103 +0x31
launchpad.net/juju-core/worker.(*notifyWorker).loop(0xc2003fc5c0, 0x0, 0x0)
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:86 +0x7e
launchpad.net/juju-core/worker.func·002()
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:53 +0x4e
created by launchpad.net/juju-core/worker.NewNotifyWorker
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:54 +0xa1

goroutine 1 [chan receive]:
testing.RunTests(0xd998b8, 0x1229700, 0x2, 0x2, 0xc2001af701, ...)
 /usr/lib/go/src/pkg/testing/testing.go:434 +0x88e
testing.Main(0xd998b8, 0x1229700, 0x2, 0x2, 0x1233ea0, ...)
 /usr/lib/go/src/pkg/testing/testing.go:365 +0x8a
main.main()
 launchpad.net/juju-core/cmd/jujud/_test/_testmain.go:45 +0x9a

goroutine 2 [syscall]:

goroutine 5 [chan receive]:
launchpad.net/juju-core/provider/dummy.func·001()
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:177 +0x3a
created by launchpad.net/juju-core/provider/dummy.init·1
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:179 +0xc1

goroutine 7 [chan receive]:
launchpad.net/gocheck.(*suiteRunner).runTest(0xc2004689a0, 0xc20046c3f0, 0xc200697140)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:771 +0x4f
launchpad.net/gocheck.(*suiteRunner).run(0xc2004689a0, 0xc2001dc280)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:584 +0x1c5
launchpad.net/gocheck.Run(0xc21fa0, 0xc2001dc280, 0xc20025d340, 0xc200393400)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:76 +0x47
launchpad.net/gocheck.RunAll(0xc20025d340, 0xc20025d340)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:68 +0xc2
launchpad.net/gocheck.TestingT(0xc2001af870)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:56 +0x2a2
launchpad.net/juju-c...

Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (16.4 KiB)

The attempt to merge lp:~rogpeppe/juju-core/377-fix-server-error into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.757s
ok launchpad.net/juju-core/agent/tools 0.252s
ok launchpad.net/juju-core/bzr 6.454s
ok launchpad.net/juju-core/cert 2.251s
ok launchpad.net/juju-core/charm 0.561s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.020s
ok launchpad.net/juju-core/cmd 0.263s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 111.567s

----------------------------------------------------------------------
FAIL: machine_test.go:377: MachineSuite.TestManageStateServesAPI

[LOG] 81.15455 INFO juju environs/testing: uploading FAKE tools 1.13.3-precise-amd64
[LOG] 81.17106 INFO juju.environs.boostrap bootstrapping environment "dummyenv"
[LOG] 81.17109 INFO juju.environs.tools reading tools with major version 1
[LOG] 81.17111 INFO juju.environs.tools filtering tools by version: 1.13.3
[LOG] 81.17112 INFO juju.environs.tools filtering tools by series: precise
[LOG] 81.17113 DEBUG juju.environs.tools reading v1.* tools
[LOG] 81.17114 INFO juju.environs.tools falling back to public bucket
[LOG] 81.17115 DEBUG juju.environs.tools reading v1.* tools
[LOG] 81.17119 DEBUG juju.environs.tools found 1.13.3-precise-amd64
[LOG] 81.17123 INFO juju environs/dummy: would pick tools from 1.13.3-precise-amd64
[LOG] 81.19443 INFO juju.state opening state; mongo addresses: ["localhost:53646"]; entity ""
[LOG] 81.19747 INFO juju.state connection established
[LOG] 81.21240 INFO juju.state initializing environment
[LOG] 81.24281 INFO juju state/api: listening on "127.0.0.1:45956"
[LOG] 81.26227 INFO juju.state opening state; mongo addresses: ["localhost:53646"]; entity ""
[LOG] 81.28891 INFO juju.state connection established
[LOG] 81.29111 INFO juju juju: authorization error while connecting to state server; retrying
[LOG] 81.29136 INFO juju.state opening state; mongo addresses: ["localhost:53646"]; entity ""
[LOG] 81.29599 INFO juju.state connection established
[LOG] 81.32019 INFO juju state/api: dialing "wss://127.0.0.1:45956/"
[LOG] 81.32306 INFO juju state/api: connection established
[LOG] 81.32317 DEBUG juju rpc/jsoncodec: <- {"RequestId":1,"Type":"Admin","Request":"Login","Params":{"AuthTag":"user-admin","Password":"dummy-secret","Nonce":""}}
[LOG] 81.32349 DEBUG juju rpc/jsoncodec: -> {"RequestId":1,"Response":{}}
[LOG] 81.32375 INFO juju.container.lxc lxcObjectFactory replaced with mock lxc factory
[LOG] 81.32485 DEBUG juju rpc/jsoncodec: <- {"RequestId":2,"Type":"Pinger","Request":"Ping","Params":{}}
[LOG] 81.32492 DEBUG juju rpc/jsoncodec: -> {"RequestId":2,"Response":{}}
[LOG] 81.39595 INFO juju machine agent machine-0 start
[LOG] 81.41037 INFO juju Starting StateWorker for machine-0
[LOG] 81.41041 INFO juju worker: start "state"
[LOG] 81.41044 INFO juju.state opening state; mongo addresses: ["localhost:53646"]; entity "machine-0"
[LOG] 81.41067 INFO juju worker: start "api"
[LOG] 81.41088 IN...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (15.7 KiB)

The attempt to merge lp:~rogpeppe/juju-core/377-fix-server-error into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.717s
ok launchpad.net/juju-core/agent/tools 0.243s
ok launchpad.net/juju-core/bzr 6.566s
ok launchpad.net/juju-core/cert 1.995s
ok launchpad.net/juju-core/charm 0.564s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.019s
ok launchpad.net/juju-core/cmd 0.260s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 111.509s

----------------------------------------------------------------------
FAIL: machine_test.go:377: MachineSuite.TestManageStateServesAPI

[LOG] 72.05873 INFO juju environs/testing: uploading FAKE tools 1.13.3-precise-amd64
[LOG] 72.07528 INFO juju.environs.boostrap bootstrapping environment "dummyenv"
[LOG] 72.07530 INFO juju.environs.tools reading tools with major version 1
[LOG] 72.07532 INFO juju.environs.tools filtering tools by version: 1.13.3
[LOG] 72.07533 INFO juju.environs.tools filtering tools by series: precise
[LOG] 72.07534 DEBUG juju.environs.tools reading v1.* tools
[LOG] 72.07535 INFO juju.environs.tools falling back to public bucket
[LOG] 72.07536 DEBUG juju.environs.tools reading v1.* tools
[LOG] 72.07540 DEBUG juju.environs.tools found 1.13.3-precise-amd64
[LOG] 72.07543 INFO juju environs/dummy: would pick tools from 1.13.3-precise-amd64
[LOG] 72.09285 INFO juju.state opening state; mongo addresses: ["localhost:54335"]; entity ""
[LOG] 72.09562 INFO juju.state connection established
[LOG] 72.13277 INFO juju.state initializing environment
[LOG] 72.15874 INFO juju state/api: listening on "127.0.0.1:55356"
[LOG] 72.18266 INFO juju.state opening state; mongo addresses: ["localhost:54335"]; entity ""
[LOG] 72.20454 INFO juju.state connection established
[LOG] 72.20567 INFO juju juju: authorization error while connecting to state server; retrying
[LOG] 72.20576 INFO juju.state opening state; mongo addresses: ["localhost:54335"]; entity ""
[LOG] 72.20930 INFO juju.state connection established
[LOG] 72.24356 INFO juju state/api: dialing "wss://127.0.0.1:55356/"
[LOG] 72.25878 INFO juju state/api: connection established
[LOG] 72.25890 DEBUG juju rpc/jsoncodec: <- {"RequestId":1,"Type":"Admin","Request":"Login","Params":{"AuthTag":"user-admin","Password":"dummy-secret","Nonce":""}}
[LOG] 72.25930 DEBUG juju rpc/jsoncodec: -> {"RequestId":1,"Response":{}}
[LOG] 72.25954 INFO juju.container.lxc lxcObjectFactory replaced with mock lxc factory
[LOG] 72.26078 DEBUG juju rpc/jsoncodec: <- {"RequestId":2,"Type":"Pinger","Request":"Ping","Params":{}}
[LOG] 72.26084 DEBUG juju rpc/jsoncodec: -> {"RequestId":2,"Response":{}}
[LOG] 72.34551 INFO juju machine agent machine-0 start
[LOG] 72.35922 INFO juju Starting StateWorker for machine-0
[LOG] 72.35927 INFO juju worker: start "state"
[LOG] 72.35929 INFO juju.state opening state; mongo addresses: ["localhost:54335"]; entity "machine-0"
[LOG] 72.35955 INFO juju worker: start "api"
[LOG] 72.35977 IN...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/apiserver/common/errors.go'
2--- state/apiserver/common/errors.go 2013-08-06 17:09:11 +0000
3+++ state/apiserver/common/errors.go 2013-08-28 09:13:33 +0000
4@@ -51,6 +51,17 @@
5 ErrNotProvisioned: params.CodeNotProvisioned,
6 }
7
8+func singletonCode(err error) (string, bool) {
9+ // All error types may not be hashable; deal with
10+ // that by catching the panic if we try to look up
11+ // a non-hashable type.
12+ defer func() {
13+ recover()
14+ }()
15+ code, ok := singletonErrorCodes[err]
16+ return code, ok
17+}
18+
19 // ServerError returns an error suitable for returning to an API
20 // client, with an error code suitable for various kinds of errors
21 // generated in packages outside the API.
22@@ -58,9 +69,9 @@
23 if err == nil {
24 return nil
25 }
26- code := singletonErrorCodes[err]
27+ code, ok := singletonCode(err)
28 switch {
29- case code != "":
30+ case ok:
31 case errors.IsUnauthorizedError(err):
32 code = params.CodeUnauthorized
33 case errors.IsNotFoundError(err):
34
35=== renamed file 'state/apiserver/errors_test.go' => 'state/apiserver/common/errors_test.go'
36--- state/apiserver/errors_test.go 2013-08-19 11:20:02 +0000
37+++ state/apiserver/common/errors_test.go 2013-08-28 09:13:33 +0000
38@@ -1,7 +1,7 @@
39 // Copyright 2012, 2013 Canonical Ltd.
40 // Licensed under the AGPLv3, see LICENCE file for details.
41
42-package apiserver_test
43+package common_test
44
45 import (
46 stderrors "errors"
47@@ -71,10 +71,19 @@
48 err: stderrors.New("an error"),
49 code: "",
50 }, {
51+ err: unhashableError{"foo"},
52+ code: "",
53+}, {
54 err: nil,
55 code: "",
56 }}
57
58+type unhashableError []string
59+
60+func (err unhashableError) Error() string {
61+ return err[0]
62+}
63+
64 func (s *errorsSuite) TestErrorTransform(c *gc.C) {
65 for _, t := range errorTransformTests {
66 err1 := common.ServerError(t.err)

Subscribers

People subscribed via source and target branches

to status/vote changes: