Skip to content

Commit

Permalink
draft data race solution for message
Browse files Browse the repository at this point in the history
  • Loading branch information
alovak committed Sep 21, 2023
1 parent b652df4 commit 4b3d7eb
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 4 deletions.
52 changes: 48 additions & 4 deletions message.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"regexp"
"sort"
"strconv"
"sync"

"github.com/moov-io/iso8583/field"
"github.com/moov-io/iso8583/utils"
Expand All @@ -28,6 +29,9 @@ type Message struct {
// stores all fields according to the spec
fields map[int]field.Field

// to guard fieldsMap
mu sync.Mutex

// tracks which fields were set
fieldsMap map[int]struct{}
}
Expand Down Expand Up @@ -62,13 +66,19 @@ func (m *Message) Bitmap() *field.Bitmap {
// it exists and is of the correct type
m.bitmap, _ = m.fields[bitmapIdx].(*field.Bitmap)
m.bitmap.Reset()

m.mu.Lock()
m.fieldsMap[bitmapIdx] = struct{}{}
m.mu.Unlock()

return m.bitmap
}

func (m *Message) MTI(val string) {
m.mu.Lock()
m.fieldsMap[mtiIdx] = struct{}{}
m.mu.Unlock()

m.fields[mtiIdx].SetBytes([]byte(val))
}

Expand All @@ -78,15 +88,20 @@ func (m *Message) GetSpec() *MessageSpec {

func (m *Message) Field(id int, val string) error {
if f, ok := m.fields[id]; ok {
m.mu.Lock()
m.fieldsMap[id] = struct{}{}
m.mu.Unlock()
return f.SetBytes([]byte(val))
}
return fmt.Errorf("failed to set field %d. ID does not exist", id)
}

func (m *Message) BinaryField(id int, val []byte) error {
if f, ok := m.fields[id]; ok {
m.mu.Lock()
m.fieldsMap[id] = struct{}{}
m.mu.Unlock()

return f.SetBytes(val)
}
return fmt.Errorf("failed to set binary field %d. ID does not exist", id)
Expand All @@ -99,15 +114,21 @@ func (m *Message) GetMTI() (string, error) {

func (m *Message) GetString(id int) (string, error) {
if f, ok := m.fields[id]; ok {
m.mu.Lock()
m.fieldsMap[id] = struct{}{}
m.mu.Unlock()

return f.String()
}
return "", fmt.Errorf("failed to get string for field %d. ID does not exist", id)
}

func (m *Message) GetBytes(id int) ([]byte, error) {
if f, ok := m.fields[id]; ok {
m.mu.Lock()
m.fieldsMap[id] = struct{}{}
m.mu.Unlock()

return f.Bytes()
}
return nil, fmt.Errorf("failed to get bytes for field %d. ID does not exist", id)
Expand All @@ -119,6 +140,9 @@ func (m *Message) GetField(id int) field.Field {

// Fields returns the map of the set fields
func (m *Message) GetFields() map[int]field.Field {
m.mu.Lock()
defer m.mu.Unlock()

fields := map[int]field.Field{}
for i := range m.fieldsMap {
fields[i] = m.GetField(i)
Expand Down Expand Up @@ -192,11 +216,19 @@ func (m *Message) Unpack(src []byte) error {
func (m *Message) unpack(src []byte) error {
var off int

m.mu.Lock()
// reset fields that were set
m.fieldsMap = map[int]struct{}{}
// we unlock here as m.Bitmap() will lock the mutex again
m.mu.Unlock()

bitmap := m.Bitmap()
// This method implicitly also sets m.fieldsMap[bitmapIdx]
m.Bitmap().Reset()
bitmap.Reset()

// lock the mutex again as we're going to set fields
m.mu.Lock()
defer m.mu.Unlock()

read, err := m.fields[mtiIdx].Unpack(src)
if err != nil {
Expand All @@ -215,13 +247,13 @@ func (m *Message) unpack(src []byte) error {

off += read

for i := 2; i <= m.Bitmap().Len(); i++ {
for i := 2; i <= bitmap.Len(); i++ {
// skip bitmap presence bits (for default bitmap length of 64 these are bits 1, 65, 129, 193, etc.)
if m.Bitmap().IsBitmapPresenceBit(i) {
if bitmap.IsBitmapPresenceBit(i) {
continue
}

if m.Bitmap().IsSet(i) {
if bitmap.IsSet(i) {
fl, ok := m.fields[i]
if !ok {
return fmt.Errorf("failed to unpack field %d: no specification found", i)
Expand Down Expand Up @@ -264,6 +296,9 @@ func (m *Message) MarshalJSON() ([]byte, error) {
}

func (m *Message) UnmarshalJSON(b []byte) error {
m.mu.Lock()
defer m.mu.Unlock()

var data map[string]json.RawMessage
if err := json.Unmarshal(b, &data); err != nil {
return err
Expand Down Expand Up @@ -291,6 +326,9 @@ func (m *Message) UnmarshalJSON(b []byte) error {
}

func (m *Message) packableFieldIDs() ([]int, error) {
m.mu.Lock()
defer m.mu.Unlock()

// Index 1 represent bitmap which is always populated.
populatedFieldIDs := []int{1}

Expand Down Expand Up @@ -338,6 +376,9 @@ func (m *Message) Clone() (*Message, error) {
// through the message fields and calls Unmarshal(...) on them setting the v If
// v is not a struct or not a pointer to struct then it returns error.
func (m *Message) Marshal(v interface{}) error {
m.mu.Lock()
defer m.mu.Unlock()

if v == nil {
return nil
}
Expand Down Expand Up @@ -391,6 +432,9 @@ func (m *Message) Marshal(v interface{}) error {
// through the message fields and calls Unmarshal(...) on them setting the v If
// v is nil or not a pointer it returns error.
func (m *Message) Unmarshal(v interface{}) error {
m.mu.Lock()
defer m.mu.Unlock()

rv := reflect.ValueOf(v)
if rv.Kind() != reflect.Ptr || rv.IsNil() {
return errors.New("data is not a pointer or nil")
Expand Down
30 changes: 30 additions & 0 deletions message_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ package iso8583
import (
"encoding/hex"
"encoding/json"
"log"
"net/http"
"reflect"
"sync"
"testing"
"time"

Expand All @@ -14,9 +17,15 @@ import (
"github.com/moov-io/iso8583/sort"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

_ "net/http/pprof"
)

func TestMessage(t *testing.T) {
go func() {
log.Println(http.ListenAndServe("localhost:6060", nil))
}()

spec := &MessageSpec{
Fields: map[int]field.Field{
0: field.NewString(&field.Spec{
Expand Down Expand Up @@ -90,6 +99,27 @@ func TestMessage(t *testing.T) {
},
}

// this test most probably will fail in regular mode,
// and should fail when is run with -race flag
t.Run("No data race when accessing fields concurrently", func(t *testing.T) {
message := NewMessage(spec)

var wg sync.WaitGroup

for i := 0; i < 1000; i++ {
wg.Add(1)
go func() {
defer wg.Done()

// calling GetString writes into the map of the
// set fields
message.GetString(0)
}()
}

wg.Wait()
})

t.Run("Test packing and unpacking untyped fields", func(t *testing.T) {
message := NewMessage(spec)
message.MTI("0100")
Expand Down

0 comments on commit 4b3d7eb

Please sign in to comment.