Skip to content

Commit

Permalink
fix: get rid of data race in encoder and fix concurrent map access
Browse files Browse the repository at this point in the history
Fixes: #3377, #3380

Fixed the data race in the encoder documentation examples by using `sync.Once`.
We only need to generate them once anyways and then it's not a big deal
that we are using the same pointers everywhere as they're pretty much
constant.

As of `system.go`, looks like we actually have concurrent operations for
partitions unmount so I just added a mutex there.

Signed-off-by: Artem Chernyshev <artem.0xD2@gmail.com>
  • Loading branch information
Unix4ever authored and talos-bot committed Mar 29, 2021
1 parent 4b3580a commit 0328518
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 64 deletions.
29 changes: 22 additions & 7 deletions internal/pkg/mount/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package mount
import (
"fmt"
"os"
"sync"

"github.com/talos-systems/go-blockdevice/blockdevice"
"github.com/talos-systems/go-blockdevice/blockdevice/filesystem"
Expand All @@ -20,7 +21,10 @@ import (
"github.com/talos-systems/talos/pkg/machinery/constants"
)

var mountpoints = map[string]*Point{}
var (
mountpoints = map[string]*Point{}
mountpointsMutex sync.RWMutex
)

// SystemMountPointsForDevice returns the mountpoints required to boot the system.
// This function is called exclusively during installations ( both image
Expand Down Expand Up @@ -195,21 +199,32 @@ func SystemPartitionMount(r runtime.Runtime, label string, opts ...Option) (err
return err
}

mountpointsMutex.Lock()
defer mountpointsMutex.Unlock()

mountpoints[label] = mountpoint

return nil
}

// SystemPartitionUnmount unmounts a system partition by the label.
func SystemPartitionUnmount(r runtime.Runtime, label string) (err error) {
if mountpoint, ok := mountpoints[label]; ok {
err = mountpoint.Unmount()
if err != nil {
return err
}
mountpointsMutex.RLock()
mountpoint, ok := mountpoints[label]
mountpointsMutex.RUnlock()

delete(mountpoints, label)
if !ok {
return nil
}

err = mountpoint.Unmount()
if err != nil {
return err
}

mountpointsMutex.Lock()
delete(mountpoints, label)
mountpointsMutex.Unlock()

return nil
}
84 changes: 58 additions & 26 deletions pkg/machinery/config/encoder/documentation.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"reflect"
"regexp"
"strings"
"sync"

yaml "gopkg.in/yaml.v3"
)
Expand Down Expand Up @@ -51,7 +52,7 @@ func (d *Doc) AddExample(name string, value interface{}) {

d.Examples = append(d.Examples, &Example{
Name: name,
Value: value,
value: value,
})
}

Expand All @@ -74,8 +75,43 @@ func (d *Doc) Describe(field string, short bool) string {

// Example represents one example snippet for a type.
type Example struct {
Name string
Value interface{}
populate sync.Once
Name string

valueMutex sync.RWMutex
value interface{}
}

// Populate populates example value.
func (e *Example) Populate(index int) {
e.populate.Do(func() {
if reflect.TypeOf(e.value).Kind() != reflect.Ptr {
return
}

v := reflect.ValueOf(e.value).Elem()

defaultValue := getExample(v, getDoc(e.value), index)

e.valueMutex.Lock()
defer e.valueMutex.Unlock()

if defaultValue != nil {
v.Set(defaultValue.Convert(v.Type()))
}

populateNestedExamples(v, index)
})
}

// GetValue returns example value.
func (e *Example) GetValue() interface{} {
e.valueMutex.RLock()
defer func() {
e.valueMutex.RUnlock()
}()

return e.value
}

// Field gets field from the list of fields.
Expand Down Expand Up @@ -168,7 +204,7 @@ func renderExample(key string, doc *Doc) string {
examples := []string{}

for i, e := range doc.Examples {
v := reflect.ValueOf(e.Value)
v := reflect.ValueOf(e.GetValue())

if !isSet(v) {
continue
Expand All @@ -179,7 +215,8 @@ func renderExample(key string, doc *Doc) string {
}

defaultValue := v.Interface()
populateExamples(defaultValue, i)

e.Populate(i)

node, err := toYamlNode(defaultValue)
if err != nil {
Expand Down Expand Up @@ -229,61 +266,56 @@ func renderExample(key string, doc *Doc) string {
return strings.Join(examples, "")
}

func readExample(v reflect.Value, doc *Doc, index int) {
func getExample(v reflect.Value, doc *Doc, index int) *reflect.Value {
if doc == nil || len(doc.Examples) == 0 {
return
return nil
}

numExamples := len(doc.Examples)
if index >= numExamples {
index = numExamples - 1
}

defaultValue := reflect.ValueOf(doc.Examples[index].Value)
defaultValue := reflect.ValueOf(doc.Examples[index].GetValue())
if isSet(defaultValue) {
if v.Kind() != reflect.Ptr && defaultValue.Kind() == reflect.Ptr {
defaultValue = defaultValue.Elem()
}

v.Set(defaultValue.Convert(v.Type()))
}

return &defaultValue
}

//nolint:gocyclo
func populateExamples(in interface{}, index int) {
doc := getDoc(in)

if reflect.TypeOf(in).Kind() != reflect.Ptr {
return
}

v := reflect.ValueOf(in).Elem()

readExample(v, doc, index)

func populateNestedExamples(v reflect.Value, index int) {
//nolint:exhaustive
switch v.Kind() {
case reflect.Struct:
doc := getDoc(v.Interface())

for i := 0; i < v.NumField(); i++ {
field := v.Field(i)
if !field.CanInterface() {
continue
}

if doc != nil && i < len(doc.Fields) {
readExample(field, doc.Field(i), index)
defaultValue := getExample(field, doc.Field(i), index)

if defaultValue != nil {
field.Set(defaultValue.Convert(field.Type()))
}
}

value := field.Interface()
populateExamples(value, index)
populateNestedExamples(field, index)
}
case reflect.Map:
for _, key := range v.MapKeys() {
populateExamples(v.MapIndex(key), index)
populateNestedExamples(v.MapIndex(key), index)
}
case reflect.Slice:
for i := 0; i < v.Len(); i++ {
populateExamples(v.Index(i), index)
populateNestedExamples(v.Index(i), index)
}
}
}
81 changes: 52 additions & 29 deletions pkg/machinery/config/encoder/encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package encoder_test

import (
"sync"
"testing"

"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -94,40 +95,25 @@ func init() {
mixinDoc.Fields = make([]encoder.Doc, 1)
mixinDoc.Fields[0].Comments[encoder.LineComment] = "was inlined"

machineDoc.Examples = []*encoder.Example{
{
Name: "uncomment me",
Value: &Machine{
State: 100,
},
},
{
Name: "second example",
Value: &Machine{
State: -1,
},
},
}
machineDoc.AddExample("uncomment me", &Machine{
State: 100,
})
machineDoc.AddExample("second example", &Machine{
State: -1,
})

machineDoc.Fields = make([]encoder.Doc, 2)
machineDoc.Fields[1].Examples = []*encoder.Example{
{
Name: "",
Value: &MachineConfig{
Version: "0.0.2",
},
},
}
machineDoc.Fields[1].AddExample("", &MachineConfig{
Version: "0.0.2",
})

machineConfigDoc.Fields = make([]encoder.Doc, 2)
machineConfigDoc.Fields[0].Comments[encoder.HeadComment] = "this is some version"
machineConfigDoc.Fields[1].Examples = []*encoder.Example{
{
Name: "",
Value: []string{
"reboot", "upgrade",
},
machineConfigDoc.Fields[1].AddExample("",
[]string{
"reboot", "upgrade",
},
}
)
}

func (c Config) Doc() *encoder.Doc {
Expand Down Expand Up @@ -329,6 +315,23 @@ mixed_in: a # was inlined
# capabilities:
# - reboot
# - upgrade
`,
incompatible: true,
},
{
name: "populate map element's examples",
value: map[string][]*MachineConfig{
"first": {
{},
},
},
expectedYAML: `first:
- # this is some version
version: ""
# capabilities:
# - reboot
# - upgrade
`,
incompatible: true,
},
Expand Down Expand Up @@ -361,6 +364,26 @@ mixed_in: a # was inlined
}
}

func (suite *EncoderSuite) TestConcurrent() {
value := &Machine{}

var wg sync.WaitGroup

for i := 0; i < 10; i++ {
wg.Add(1)

go func() {
defer wg.Done()

encoder := encoder.NewEncoder(value)
_, err := encoder.Encode()
suite.Assert().NoError(err)
}()
}

wg.Wait()
}

func decodeToMap(data []byte) (map[interface{}]interface{}, error) {
raw := map[interface{}]interface{}{}
err := yaml.Unmarshal(data, &raw)
Expand Down
4 changes: 2 additions & 2 deletions pkg/machinery/config/encoder/markdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ var markdownTemplate = `
Examples:
{{ range $example := .Examples }}
{{ yaml $example.Value $.Name }}
{{ yaml $example.GetValue $.Name }}
{{ end }}
{{ end }}
Expand All @@ -43,7 +43,7 @@ Appears in:
{{ if $struct.Examples -}}
{{ range $example := $struct.Examples }}
{{ yaml $example.Value "" }}
{{ yaml $example.GetValue "" }}
{{- end -}}
{{ end }}
Expand Down

0 comments on commit 0328518

Please sign in to comment.