Merge lp://staging/~dave-cheney/juju-core/110-fix-lp-1251076 into lp://staging/~go-bot/juju-core/trunk

Proposed by Dave Cheney
Status: Merged
Approved by: Dave Cheney
Approved revision: no longer in the source branch.
Merged at revision: 2550
Proposed branch: lp://staging/~dave-cheney/juju-core/110-fix-lp-1251076
Merge into: lp://staging/~go-bot/juju-core/trunk
Diff against target: 18 lines (+7/-1)
1 file modified
rpc/rpcreflect/type.go (+7/-1)
To merge this branch: bzr merge lp://staging/~dave-cheney/juju-core/110-fix-lp-1251076
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+213764@code.staging.launchpad.net

Commit message

Workaround LP 1251076

gccgo appears to get confused and thinks that out[1] is not nil when in fact it is an interface value of type error and value nil.

This workaround solves the problem by leaving error as nil if in fact it was nil and causes no harm for gc because the predicates above assert that if out[1] is not nil, then it is an error type.

https://codereview.appspot.com/83340045/

Description of the change

Workaround LP 1251076

gccgo appears to get confused and thinks that out[1] is not nil when in fact it is an interface value of type error and value nil.

This workaround solves the problem by leaving error as nil if in fact it was nil and causes no harm for gc because the predicates above assert that if out[1] is not nil, then it is an error type.

https://codereview.appspot.com/83340045/

To post a comment you must log in.
Revision history for this message
Dave Cheney (dave-cheney) wrote :

Reviewers: mp+213764_code.launchpad.net,

Message:
Please take a look.

Description:
Workaround LP 1251076

gccgo appears to get confused and things that out[1] is not nil when in
fact it is an interface value of type error and value nil.

This workaround solves the problem by leaving error as nil if in fact it
was nil and causes no harm for gc because the predicates above assert
that if out[1] is not nil, then it is an error type.

https://code.launchpad.net/~dave-cheney/juju-core/110-fix-lp-1251076/+merge/213764

(do not edit description out of merge proposal)

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

Affected files (+9, -1 lines):
   A [revision details]
   M rpc/rpcreflect/type.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-20140402024520-p4uc9yiaf00lnjcy
+New revision: <email address hidden>

Index: rpc/rpcreflect/type.go
=== modified file 'rpc/rpcreflect/type.go'
--- rpc/rpcreflect/type.go 2013-09-24 07:12:05 +0000
+++ rpc/rpcreflect/type.go 2014-04-02 04:55:38 +0000
@@ -144,7 +144,13 @@
   f := func(rcvr reflect.Value, id string) (r reflect.Value, err error) {
    out := rcvr.Method(m.Index).Call([]reflect.Value{reflect.ValueOf(id)})
    if !out[1].IsNil() {
- err = out[1].Interface().(error)
+ // Workaround LP 1251076.
+ // gccgo appears to get confused and things that out[1] is not nil when
+ // in fact it is an interface value of type error and value nil.
+ // This workaround solves the problem by leaving error as nil if in fact
+ // it was nil and causes no harm for gc because the predicates above
+ // assert that if out[1] is not nil, then it is an error type.
+ err, _ = out[1].Interface().(error)
    }
    r = out[0]
    return

Revision history for this message
Dave Cheney (dave-cheney) wrote :
Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Dave Cheney (dave-cheney) wrote :

Thanks William. I'd like to hear from Rog as well as I stared at this
one for several hours this afternoon.

On Wed, Apr 2, 2014 at 6:21 PM, William Reade
<email address hidden> wrote:
> LGTM
>
> https://codereview.appspot.com/83340045/
>
> --
> https://code.launchpad.net/~dave-cheney/juju-core/110-fix-lp-1251076/+merge/213764
> You are the owner of lp:~dave-cheney/juju-core/110-fix-lp-1251076.

Revision history for this message
Dave Cheney (dave-cheney) wrote :

Roger, last chance to comment before I submit this thing so I can get
the rpc tests passing on ppc64

On Wed, Apr 2, 2014 at 6:33 PM, David Cheney <email address hidden> wrote:
> Thanks William. I'd like to hear from Rog as well as I stared at this
> one for several hours this afternoon.
>
> On Wed, Apr 2, 2014 at 6:21 PM, William Reade
> <email address hidden> wrote:
>> LGTM
>>
>> https://codereview.appspot.com/83340045/
>>
>> --
>> https://code.launchpad.net/~dave-cheney/juju-core/110-fix-lp-1251076/+merge/213764
>> You are the owner of lp:~dave-cheney/juju-core/110-fix-lp-1251076.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: