fix: get rid of data race in encoder and fix concurrent map access

Fixes: https://github.com/talos-systems/talos/issues/3377, https://github.com/talos-systems/talos/issues/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>
This commit is contained in:
Artem Chernyshev 2021-03-26 17:43:13 +03:00 committed by talos-bot
parent 4b3580aa57
commit 032851844f
4 changed files with 134 additions and 64 deletions

View File

@ -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"
@ -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
@ -195,6 +199,9 @@ func SystemPartitionMount(r runtime.Runtime, label string, opts ...Option) (err
return err
}
mountpointsMutex.Lock()
defer mountpointsMutex.Unlock()
mountpoints[label] = mountpoint
return nil
@ -202,14 +209,22 @@ func SystemPartitionMount(r runtime.Runtime, label string, opts ...Option) (err
// 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
}

View File

@ -8,6 +8,7 @@ import (
"reflect"
"regexp"
"strings"
"sync"
yaml "gopkg.in/yaml.v3"
)
@ -51,7 +52,7 @@ func (d *Doc) AddExample(name string, value interface{}) {
d.Examples = append(d.Examples, &Example{
Name: name,
Value: value,
value: value,
})
}
@ -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.
@ -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
@ -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 {
@ -229,9 +266,9 @@ 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)
@ -239,31 +276,23 @@ func readExample(v reflect.Value, doc *Doc, index int) {
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() {
@ -271,19 +300,22 @@ func populateExamples(in interface{}, index int) {
}
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)
}
}
}

View File

@ -5,6 +5,7 @@
package encoder_test
import (
"sync"
"testing"
"github.com/stretchr/testify/suite"
@ -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 {
@ -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,
},
@ -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)

View File

@ -21,7 +21,7 @@ var markdownTemplate = `
Examples:
{{ range $example := .Examples }}
{{ yaml $example.Value $.Name }}
{{ yaml $example.GetValue $.Name }}
{{ end }}
{{ end }}
@ -43,7 +43,7 @@ Appears in:
{{ if $struct.Examples -}}
{{ range $example := $struct.Examples }}
{{ yaml $example.Value "" }}
{{ yaml $example.GetValue "" }}
{{- end -}}
{{ end }}