Merge pull request #35 from georgepsarakis/fix-iso3166-race-condition

Fix data race when calling `GetISO3166`
dongri · Oct 4, 2023 · 97a2d6b · 97a2d6b
2 parents 300a6e9 + b3ac1c1
commit 97a2d6b
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 58 deletions.
diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml
@@ -22,4 +22,4 @@ jobs:
           go-version: 1.19
 
       - name: Test
-        run: go test -v
+        run: go test -v -race
diff --git a/iso3166.go b/iso3166.go
@@ -10,15 +10,27 @@ type ISO3166 struct {
 	PhoneNumberLengths []int
 }
 
+func init() {
+	// Run once during package initialization in order to avoid data races
+	// https://go.dev/doc/effective_go#init
+	populateISO3166()
+}
+
 var iso3166Datas []ISO3166
 
-// GetISO3166 ...
+// GetISO3166 returns the ISO3166 configuration for each country.
+// Data are loaded during package initialization.
 func GetISO3166() []ISO3166 {
+	return iso3166Datas
+}
+
+// populateISO3166 contains the definitions of the per-country mobile number configuration.
+// It operates on the iso3166Datas global variable and will return it after population.
+func populateISO3166() {
 	if iso3166Datas != nil {
-		return iso3166Datas
+		return
 	}
 
-	iso3166Datas = []ISO3166{}
 	var i = ISO3166{}
 
 	i.Alpha2 = "US"
@@ -1871,6 +1883,4 @@ func GetISO3166() []ISO3166 {
 	i.MobileBeginWith = []string{"71", "73", "77"}
 	i.PhoneNumberLengths = []int{9}
 	iso3166Datas = append(iso3166Datas, i)
-
-	return iso3166Datas
 }
diff --git a/phonenumber_test.go b/phonenumber_test.go
@@ -4,6 +4,54 @@ import (
 	"testing"
 )
 
+// Get country by mobile number only
+var mobWithLLCountryTests = []struct {
+	input    string
+	expected string
+}{
+	// landline numbers
+	{"3726347343", "EE"},
+	{"74997098833", "RU"},
+	{"37167881727", "LV"},
+	{"16466909997", "US"},
+	{"14378869667", "CA"},
+	{"12836907618", "US"},
+	{"13406407159", "VI"},
+	{"5117061970", "PE"},
+	{"862185551232", "CN"},
+	{"38391234999", "XK"},
+
+	// Mobile numbers
+	{"39339638066", "IT"},
+	{"37125641580", "LV"},
+	{"43663242739", "AT"},
+	{"21655886170", "TN"},
+	{"3197010280754", "NL"},
+	{"51999400500", "PE"},
+	{"8614855512329", "CN"},
+	{"38342224999", "XK"},
+}
+
+func TestGetCountryForMobileNumberWithLandLine(t *testing.T) {
+	for _, tt := range mobWithLLCountryTests {
+		tt := tt
+		t.Run(tt.input, func(t *testing.T) {
+			t.Parallel()
+			country := GetISO3166ByNumber(tt.input, true)
+			if tt.expected == "" {
+				if country.CountryName != "" {
+					t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): must be empty, actual `%s`", tt.input, country.CountryName)
+				}
+			} else {
+				expected := getISO3166ByCountry(tt.expected)
+				if country.CountryName != expected.CountryName {
+					t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): expected `%s`, actual `%s`", tt.input, expected.CountryName, country.CountryName)
+				}
+			}
+		})
+	}
+}
+
 // Tests format mobile
 var mobFormatTests = []struct {
 	input    string
@@ -26,10 +74,14 @@ var mobFormatTests = []struct {
 
 func TestFormatMobile(t *testing.T) {
 	for _, tt := range mobFormatTests {
-		number := Parse(tt.input, tt.country)
-		if number != tt.expected {
-			t.Errorf("Parse(number=`%s`, country=`%s`): expected `%s`, actual `%s`", tt.input, tt.country, tt.expected, number)
-		}
+		tt := tt
+		t.Run(tt.input, func(t *testing.T) {
+			t.Parallel()
+			number := Parse(tt.input, tt.country)
+			if number != tt.expected {
+				t.Errorf("Parse(number=`%s`, country=`%s`): expected `%s`, actual `%s`", tt.input, tt.country, tt.expected, number)
+			}
+		})
 	}
 }
 
@@ -99,50 +151,6 @@ func TestFormatWithLandLine(t *testing.T) {
 	}
 }
 
-// Get country by mobile number only
-var mobWithLLCountryTests = []struct {
-	input    string
-	expected string
-}{
-	// landline numbers
-	{"3726347343", "EE"},
-	{"74997098833", "RU"},
-	{"37167881727", "LV"},
-	{"16466909997", "US"},
-	{"14378869667", "CA"},
-	{"12836907618", "US"},
-	{"13406407159", "VI"},
-	{"5117061970", "PE"},
-	{"862185551232", "CN"},
-	{"38391234999", "XK"},
-
-	// Mobile numbers
-	{"39339638066", "IT"},
-	{"37125641580", "LV"},
-	{"43663242739", "AT"},
-	{"21655886170", "TN"},
-	{"3197010280754", "NL"},
-	{"51999400500", "PE"},
-	{"8614855512329", "CN"},
-	{"38342224999", "XK"},
-}
-
-func TestGetCountryForMobileNumberWithLandLine(t *testing.T) {
-	for _, tt := range mobWithLLCountryTests {
-		country := GetISO3166ByNumber(tt.input, true)
-		if tt.expected == "" {
-			if country.CountryName != "" {
-				t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): must be empty, actual `%s`", tt.input, country.CountryName)
-			}
-		} else {
-			expected := getISO3166ByCountry(tt.expected)
-			if country.CountryName != expected.CountryName {
-				t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): expected `%s`, actual `%s`", tt.input, expected.CountryName, country.CountryName)
-			}
-		}
-	}
-}
-
 // Test the real and validated mobile number for India country
 // We added "910" prefix that does not match a specification, but the numbers are really exists
 var indiaMobileTests = []struct {
@@ -252,10 +260,15 @@ var indiaMobileTests = []struct {
 
 func TestIndiaMobileNumber(t *testing.T) {
 	for _, tt := range indiaMobileTests {
-		country := GetISO3166ByNumber(tt.input, false)
-		if country.CountryName != "India" {
-			t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): expected `%s`, actual `%s`", tt.input, "India", country.CountryName)
-		}
+		tt := tt
+		t.Run(tt.input, func(t *testing.T) {
+			t.Parallel()
+
+			country := GetISO3166ByNumber(tt.input, false)
+			if country.CountryName != "India" {
+				t.Errorf("GetISO3166ByNumber(number=`%s`, withLandline=false): expected `%s`, actual `%s`", tt.input, "India", country.CountryName)
+			}
+		})
 	}
 }