This is an automated email from the ASF dual-hosted git repository.
bkietz pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 62e1e9ad76 GH-38795: [Go] Fix race GetToTimeFunc for Timestamp (#38797)
62e1e9ad76 is described below
commit 62e1e9ad76493729af6d605b7c0fcb31ebcbda5a
Author: Matt Topol <[email protected]>
AuthorDate: Mon Nov 27 10:44:53 2023 -0500
GH-38795: [Go] Fix race GetToTimeFunc for Timestamp (#38797)
### Rationale for this change
Adding RWMutex to protect `loc` in `TimestampType` and fix the race
condition.
### Are these changes tested?
Yes, a unit test is added which is covered by the CI which runs with
`-race`.
### Are there any user-facing changes?
Copying `TimestampType` will now be problematic and linters will show it as
an error. In theory this shouldn't be a problem as most uses of TimestampType
should be utilizing pointers to it rather than the value directly.
* Closes: #38795
Lead-authored-by: Matt Topol <[email protected]>
Co-authored-by: Benjamin Kietzman <[email protected]>
Co-authored-by: Ben Harkins <[email protected]>
Signed-off-by: Benjamin Kietzman <[email protected]>
---
go/arrow/compare_test.go | 53 ++++++++++++++++++------------------
go/arrow/datatype_fixedwidth.go | 16 ++++++++++-
go/arrow/datatype_fixedwidth_test.go | 25 +++++++++++++++++
3 files changed, 66 insertions(+), 28 deletions(-)
diff --git a/go/arrow/compare_test.go b/go/arrow/compare_test.go
index 170fc2d852..62e30e634e 100644
--- a/go/arrow/compare_test.go
+++ b/go/arrow/compare_test.go
@@ -116,13 +116,13 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
false, true,
},
@@ -131,13 +131,13 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
false, false,
},
@@ -146,13 +146,13 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f0", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f0": []int{0}},
+ index: map[string][]int{"f0": {0}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
false, false,
},
@@ -161,14 +161,14 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
false, true,
},
@@ -177,14 +177,14 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
false, false,
},
@@ -193,13 +193,13 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
},
&StructType{
fields: []Field{
{Name: "f2", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f2": []int{0}},
+ index: map[string][]int{"f2": {0}},
},
false, false,
},
@@ -209,14 +209,14 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
true, false,
},
@@ -226,14 +226,14 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
true, false,
},
@@ -243,7 +243,7 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
meta: MetadataFrom(map[string]string{"k1":
"v1", "k2": "v2"}),
},
&StructType{
@@ -251,7 +251,7 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
meta: MetadataFrom(map[string]string{"k2":
"v2", "k1": "v1"}),
},
true, true,
@@ -261,14 +261,14 @@ func TestTypeEqual(t *testing.T) {
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
meta: MetadataFrom(map[string]string{"k1":
"v1"}),
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0}},
+ index: map[string][]int{"f1": {0}},
meta: MetadataFrom(map[string]string{"k1":
"v2"}),
},
true, false,
@@ -279,14 +279,14 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true, Metadata:
MetadataFrom(map[string]string{"k1": "v1"})},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true, Metadata:
MetadataFrom(map[string]string{"k1": "v2"})},
{Name: "f2", Type:
PrimitiveTypes.Float32, Nullable: false},
},
- index: map[string][]int{"f1": []int{0}, "f2":
[]int{1}},
+ index: map[string][]int{"f1": {0}, "f2": {1}},
},
false, true,
},
@@ -296,14 +296,14 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0, 1}},
+ index: map[string][]int{"f1": {0, 1}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0, 1}},
+ index: map[string][]int{"f1": {0, 1}},
},
true, true,
},
@@ -313,14 +313,14 @@ func TestTypeEqual(t *testing.T) {
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
},
- index: map[string][]int{"f1": []int{0, 1}},
+ index: map[string][]int{"f1": {0, 1}},
},
&StructType{
fields: []Field{
{Name: "f1", Type:
PrimitiveTypes.Uint16, Nullable: true},
{Name: "f1", Type:
PrimitiveTypes.Uint32, Nullable: true},
},
- index: map[string][]int{"f1": []int{0, 1}},
+ index: map[string][]int{"f1": {0, 1}},
},
false, true,
},
@@ -343,7 +343,6 @@ func TestTypeEqual(t *testing.T) {
MapOf(BinaryTypes.String, &TimestampType{
Unit: 0,
TimeZone: "UTC",
- loc: nil,
}),
true, false,
},
diff --git a/go/arrow/datatype_fixedwidth.go b/go/arrow/datatype_fixedwidth.go
index bcbc8ef6ae..1a3074e59e 100644
--- a/go/arrow/datatype_fixedwidth.go
+++ b/go/arrow/datatype_fixedwidth.go
@@ -19,6 +19,7 @@ package arrow
import (
"fmt"
"strconv"
+ "sync"
"time"
"github.com/apache/arrow/go/v15/internal/json"
@@ -354,6 +355,7 @@ type TimestampType struct {
TimeZone string
loc *time.Location
+ mx sync.RWMutex
}
func (*TimestampType) ID() Type { return TIMESTAMP }
@@ -386,6 +388,8 @@ func (t *TimestampType) TimeUnit() TimeUnit { return t.Unit
}
// This should be called if you change the value of the TimeZone after having
// potentially called GetZone.
func (t *TimestampType) ClearCachedLocation() {
+ t.mx.Lock()
+ defer t.mx.Unlock()
t.loc = nil
}
@@ -398,10 +402,20 @@ func (t *TimestampType) ClearCachedLocation() {
// so if you change the value of TimeZone after calling this, make sure to call
// ClearCachedLocation.
func (t *TimestampType) GetZone() (*time.Location, error) {
+ t.mx.RLock()
if t.loc != nil {
+ defer t.mx.RUnlock()
return t.loc, nil
}
+ t.mx.RUnlock()
+ t.mx.Lock()
+ defer t.mx.Unlock()
+ // in case GetZone() was called in between releasing the read lock and
+ // getting the write lock
+ if t.loc != nil {
+ return t.loc, nil
+ }
// the TimeZone string is allowed to be either a valid tzdata string
// such as "America/New_York" or an absolute offset of the form -XX:XX
// or +XX:XX
@@ -415,7 +429,7 @@ func (t *TimestampType) GetZone() (*time.Location, error) {
if loc, err := time.LoadLocation(t.TimeZone); err == nil {
t.loc = loc
- return t.loc, err
+ return loc, err
}
// at this point we know that the timezone isn't empty, and didn't match
diff --git a/go/arrow/datatype_fixedwidth_test.go
b/go/arrow/datatype_fixedwidth_test.go
index 918572d40b..b3cbb465f3 100644
--- a/go/arrow/datatype_fixedwidth_test.go
+++ b/go/arrow/datatype_fixedwidth_test.go
@@ -17,6 +17,7 @@
package arrow_test
import (
+ "sync"
"testing"
"time"
@@ -180,6 +181,30 @@ func TestTimestampType_GetToTimeFunc(t *testing.T) {
assert.Equal(t, "2345-12-29T19:00:00-05:00",
toTimeNY(ts).Format(time.RFC3339))
}
+// Test race condition from GH-38795
+func TestGetToTimeFuncRace(t *testing.T) {
+ var (
+ wg sync.WaitGroup
+ w = make(chan bool)
+ routineNum = 10
+ )
+
+ wg.Add(routineNum)
+ for i := 0; i < routineNum; i++ {
+ go func() {
+ defer wg.Done()
+
+ <-w
+
+ _, _ =
arrow.FixedWidthTypes.Timestamp_s.(*arrow.TimestampType).GetToTimeFunc()
+ }()
+ }
+
+ close(w)
+
+ wg.Wait()
+}
+
func TestTime32Type(t *testing.T) {
for _, tc := range []struct {
unit arrow.TimeUnit
