FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/


Description:
encoding/gob: fix data race in Register
Fixes issue 4214.

Please review this at http://codereview.appspot.com/6637047/

Affected files:
M src/pkg/encoding/gob/decode.go
M src/pkg/encoding/gob/encode.go
M src/pkg/encoding/gob/type.go
M src/pkg/encoding/gob/type_test.go


Index: src/pkg/encoding/gob/decode.go
===================================================================
--- a/src/pkg/encoding/gob/decode.go
+++ b/src/pkg/encoding/gob/decode.go
@@ -717,7 +717,9 @@
errorf("name too long (%d bytes): %.20q...", len(name), name)
}
// The concrete type must be registered.
+ registerLock.Lock()
typ, ok := nameToConcreteType[name]
+ registerLock.Unlock()
if !ok {
errorf("name not registered for interface: %q", name)
}
Index: src/pkg/encoding/gob/encode.go
===================================================================
--- a/src/pkg/encoding/gob/encode.go
+++ b/src/pkg/encoding/gob/encode.go
@@ -441,7 +441,9 @@
}

ut := userType(iv.Elem().Type())
+ registerLock.Lock()
name, ok := concreteTypeToName[ut.base]
+ registerLock.Unlock()
if !ok {
errorf("type not registered for interface: %s", ut.base)
}
Index: src/pkg/encoding/gob/type.go
===================================================================
--- a/src/pkg/encoding/gob/type.go
+++ b/src/pkg/encoding/gob/type.go
@@ -712,6 +712,7 @@
}

var (
+ registerLock sync.Mutex
nameToConcreteType = make(map[string]reflect.Type)
concreteTypeToName = make(map[reflect.Type]string)
)
@@ -723,6 +724,8 @@
// reserved for nil
panic("attempt to register empty name")
}
+ registerLock.Lock()
+ defer registerLock.Unlock()
ut := userType(reflect.TypeOf(value))
// Check for incompatible duplicates. The name must refer to the
// same user type, and vice versa.
Index: src/pkg/encoding/gob/type_test.go
===================================================================
--- a/src/pkg/encoding/gob/type_test.go
+++ b/src/pkg/encoding/gob/type_test.go
@@ -177,7 +177,10 @@
Register(tc.t)

tct := reflect.TypeOf(tc.t)
- if ct := nameToConcreteType[tc.name]; ct != tct {
+ registerLock.Lock()
+ ct := nameToConcreteType[tc.name]
+ registerLock.Unlock()
+ if ct != tct {
t.Errorf("nameToConcreteType[%q] = %v, want %v", tc.name, ct, tct)
}
// concreteTypeToName is keyed off the base type.

Search Discussions

  • Brad Fitzpatrick at Oct 9, 2012 at 12:32 am
    RWMutex?
    On Mon, Oct 8, 2012 at 5:24 PM, wrote:

    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com,

    I'd like you to review this change to
    https://code.google.com/p/go/


    Description:
    encoding/gob: fix data race in Register
    Fixes issue 4214.

    Please review this at http://codereview.appspot.com/**6637047/<http://codereview.appspot.com/6637047/>

    Affected files:
    M src/pkg/encoding/gob/decode.go
    M src/pkg/encoding/gob/encode.go
    M src/pkg/encoding/gob/type.go
    M src/pkg/encoding/gob/type_**test.go


    Index: src/pkg/encoding/gob/decode.go
    ==============================**==============================**=======
    --- a/src/pkg/encoding/gob/decode.**go
    +++ b/src/pkg/encoding/gob/decode.**go
    @@ -717,7 +717,9 @@
    errorf("name too long (%d bytes): %.20q...", len(name),
    name)
    }
    // The concrete type must be registered.
    + registerLock.Lock()
    typ, ok := nameToConcreteType[name]
    + registerLock.Unlock()
    if !ok {
    errorf("name not registered for interface: %q", name)
    }
    Index: src/pkg/encoding/gob/encode.go
    ==============================**==============================**=======
    --- a/src/pkg/encoding/gob/encode.**go
    +++ b/src/pkg/encoding/gob/encode.**go
    @@ -441,7 +441,9 @@
    }

    ut := userType(iv.Elem().Type())
    + registerLock.Lock()
    name, ok := concreteTypeToName[ut.base]
    + registerLock.Unlock()
    if !ok {
    errorf("type not registered for interface: %s", ut.base)
    }
    Index: src/pkg/encoding/gob/type.go
    ==============================**==============================**=======
    --- a/src/pkg/encoding/gob/type.go
    +++ b/src/pkg/encoding/gob/type.go
    @@ -712,6 +712,7 @@
    }

    var (
    + registerLock sync.Mutex
    nameToConcreteType = make(map[string]reflect.Type)
    concreteTypeToName = make(map[reflect.Type]string)
    )
    @@ -723,6 +724,8 @@
    // reserved for nil
    panic("attempt to register empty name")
    }
    + registerLock.Lock()
    + defer registerLock.Unlock()
    ut := userType(reflect.TypeOf(value)**)
    // Check for incompatible duplicates. The name must refer to the
    // same user type, and vice versa.
    Index: src/pkg/encoding/gob/type_**test.go
    ==============================**==============================**=======
    --- a/src/pkg/encoding/gob/type_**test.go
    +++ b/src/pkg/encoding/gob/type_**test.go
    @@ -177,7 +177,10 @@
    Register(tc.t)

    tct := reflect.TypeOf(tc.t)
    - if ct := nameToConcreteType[tc.name]; ct != tct {
    + registerLock.Lock()
    + ct := nameToConcreteType[tc.name]
    + registerLock.Unlock()
    + if ct != tct {
    t.Errorf("nameToConcreteType[%**q] = %v, want
    %v", tc.name, ct, tct)
    }
    // concreteTypeToName is keyed off the base type.

  • David Symonds at Oct 9, 2012 at 12:33 am
    This seems like an obvious use case for sync.RWMutex instead: an
    initial few writes, but then almost always reads after that.
  • R at Oct 9, 2012 at 12:43 am
    Hello golang-dev@googlegroups.com, dsymonds@golang.org,
    bradfitz@golang.org (cc: golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6637047/
  • Brad Fitzpatrick at Oct 9, 2012 at 12:44 am
    LGTM
    On Mon, Oct 8, 2012 at 5:43 PM, wrote:

    Hello golang-dev@googlegroups.com, dsymonds@golang.org,
    bradfitz@golang.org (cc: golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/**6637047/<http://codereview.appspot.com/6637047/>
  • Dsymonds at Oct 9, 2012 at 12:57 am
  • R at Oct 9, 2012 at 1:03 am
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=ddfae026505a ***

    encoding/gob: fix data race in Register
    Fixes issue 4214.

    R=golang-dev, dsymonds, bradfitz
    CC=golang-dev
    http://codereview.appspot.com/6637047


    http://codereview.appspot.com/6637047/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 9, '12 at 12:25a
activeOct 9, '12 at 1:03a
posts7
users3
websitegolang.org

3 users in discussion

R: 3 posts Dsymonds: 2 posts Brad Fitzpatrick: 2 posts

People

Translate

site design / logo © 2021 Grokbase