From 49e20c943305ad7ef4a39819fb60f68b8d15f73a Mon Sep 17 00:00:00 2001 From: "natan.streppel" Date: Mon, 26 Jan 2026 11:43:01 -0300 Subject: [PATCH] fix: ExpTaylor race condition --- decimal.go | 28 +++++++++----- decimal_test.go | 97 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 108 insertions(+), 17 deletions(-) diff --git a/decimal.go b/decimal.go index 3cf7f68..538f347 100644 --- a/decimal.go +++ b/decimal.go @@ -25,6 +25,7 @@ import ( "regexp" "strconv" "strings" + "sync" ) // DivisionPrecision is the number of decimal places in the result when it @@ -95,6 +96,7 @@ var tenInt = big.NewInt(10) var twentyInt = big.NewInt(20) var factorials = []Decimal{New(1, 0)} +var factorialsMutex sync.RWMutex // Decimal represents a fixed-point decimal. It is immutable. // number = value * 10 ^ exp @@ -1107,15 +1109,21 @@ func (d Decimal) ExpTaylor(precision int32) (Decimal, error) { i++ // Calculate next factorial number or retrieve cached value + factorialsMutex.RLock() if len(factorials) >= int(i) && !factorials[i-1].IsZero() { factorial = factorials[i-1] + factorialsMutex.RUnlock() } else { - // To avoid any race conditions, firstly the zero value is appended to a slice to create - // a spot for newly calculated factorial. After that, the zero value is replaced by calculated - // factorial using the index notation. - factorial = factorials[i-2].Mul(New(i, 0)) - factorials = append(factorials, Zero) - factorials[i-1] = factorial + prevFactorial := factorials[i-2] + factorialsMutex.RUnlock() + factorial = prevFactorial.Mul(New(i, 0)) + factorialsMutex.Lock() + // Check again in case another goroutine already added it. + if len(factorials) < int(i) || factorials[i-1].IsZero() { + factorials = append(factorials, Zero) + factorials[i-1] = factorial + } + factorialsMutex.Unlock() } } @@ -2332,10 +2340,10 @@ var _tanP = [...]Decimal{ } var _tanQ = [...]Decimal{ NewFromFloat(1.00000000000000000000e+0), - NewFromFloat(1.36812963470692954678e+4), //0x40cab8a5eeb36572 - NewFromFloat(-1.32089234440210967447e+6), //0xc13427bc582abc96 - NewFromFloat(2.50083801823357915839e+7), //0x4177d98fc2ead8ef - NewFromFloat(-5.38695755929454629881e+7), //0xc189afe03cbe5a31 + NewFromFloat(1.36812963470692954678e+4), // 0x40cab8a5eeb36572 + NewFromFloat(-1.32089234440210967447e+6), // 0xc13427bc582abc96 + NewFromFloat(2.50083801823357915839e+7), // 0x4177d98fc2ead8ef + NewFromFloat(-5.38695755929454629881e+7), // 0xc189afe03cbe5a31 } // Tan returns the tangent of the radian argument x. diff --git a/decimal_test.go b/decimal_test.go index 164bb30..9cce90a 100644 --- a/decimal_test.go +++ b/decimal_test.go @@ -12,6 +12,7 @@ import ( "regexp" "strconv" "strings" + "sync" "testing" "testing/quick" "time" @@ -648,7 +649,7 @@ func TestCopy(t *testing.T) { t.Error("expecting copy and origin to be equals, but they are not") } - //change value + // change value cpy = cpy.Add(New(1, 0)) if cpy.Cmp(origin) == 0 { @@ -3004,6 +3005,88 @@ func TestDecimal_ExpTaylor(t *testing.T) { } } +// TestDecimal_ExpTaylor_Concurrency tests the concurrency safety of the +// ExpTaylor method, particularly focusing on the shared state caching mechanism +// for factorial calculations. +func TestDecimal_ExpTaylor_Concurrency(t *testing.T) { + resetFactorials := func() { + factorialsMutex.Lock() + factorials = []Decimal{New(1, 0)} + factorialsMutex.Unlock() + } + t.Run("concurrent calculation works as expected", func(t *testing.T) { + resetFactorials() + + // Run multiple goroutines calculating exp concurrently + // This should trigger concurrent factorial calculations. + var wg sync.WaitGroup + numGoroutines := 50 + results := make([]Decimal, numGoroutines) + errors := make([]error, numGoroutines) + + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func(index int) { + defer wg.Done() + d := NewFromFloat(5.0) + result, err := d.ExpTaylor(10) + results[index] = result + errors[index] = err + }(i) + } + + wg.Wait() + + // Check all goroutines succeeded. + for i, err := range errors { + if err != nil { + t.Errorf("goroutine %d: ExpTaylor failed: %v", i, err) + } + } + + // All results should be identical. + expected := results[0] + for i, result := range results[1:] { + if !result.Equal(expected) { + t.Errorf("goroutine %d: result mismatch: expected %s, got %s", i+1, expected.String(), result.String()) + } + } + + // Verify factorials slice doesn't have duplicate Zero values + // (which would indicate the double-check locking failed). + factorialsMutex.RLock() + for i, f := range factorials { + if i > 0 && f.IsZero() { + t.Errorf("factorial at index %d is Zero, double-check locking may have failed", i) + } + } + factorialsMutex.RUnlock() + }) + + t.Run("race detection", func(t *testing.T) { + // NOTE: Factorials from previous subtests are intentionally reused here. + // This tests that concurrent access works correctly with a pre-populated cache, + // which is the real-world scenario. Run with `go test -race`. + + var wg sync.WaitGroup + numGoroutines := 100 + + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func(val float64) { + defer wg.Done() + d := NewFromFloat(val) + _, err := d.ExpTaylor(10) + if err != nil { + t.Errorf("ExpTaylor failed for value %f: %v", val, err) + } + }(float64(i%10) + 0.5) + } + + wg.Wait() + }) +} + func TestDecimal_Ln(t *testing.T) { for _, testCase := range []struct { Dec string @@ -3647,9 +3730,9 @@ func ExampleNewFromFloat32() { fmt.Println(NewFromFloat32(.123123123123123).String()) fmt.Println(NewFromFloat32(-1e13).String()) // OUTPUT: - //123.12312 - //0.123123124 - //-10000000000000 + // 123.12312 + // 0.123123124 + // -10000000000000 } func ExampleNewFromFloat() { @@ -3657,9 +3740,9 @@ func ExampleNewFromFloat() { fmt.Println(NewFromFloat(.123123123123123).String()) fmt.Println(NewFromFloat(-1e13).String()) // OUTPUT: - //123.123123123123 - //0.123123123123123 - //-10000000000000 + // 123.123123123123 + // 0.123123123123123 + // -10000000000000 } func TestDecimal_String(t *testing.T) {