iancoleman / strcase

A golang package for converting to snake_case or CamelCase
MIT License
1.01k stars 106 forks source link

Concurrency issue 'fatal error: concurrent map writes' #43

Closed Snookmz closed 1 year ago

Snookmz commented 1 year ago

Looks like an issue occurs when there are lots of ConfigureAcronym calls and ToCamel is called concurrently. I'm not exactly sure why this would happen but below is how to replicate the problem in a unit test:

concurrency_test.go

func TestConcurrency(t *testing.T) {
    for i := 0; i < 10000; i++ {
        go doConvert()
    }

}

func doConvert() {
    var snakes = []string{"code", "exchange", "pe_ratio", "profit_margin", "updated_date"}
    for _, v := range snakes {
        _ = convertDatabaseNameToCamelCase(v)
    }
}

func convertDatabaseNameToCamelCase(d string) (s string) {
    ConfigureAcronym("price_book_mrq", "PriceBookMRQ")
    ConfigureAcronym("pe_ratio", "PERatio")
    ConfigureAcronym("peg_ratio", "PEGRatio")
    ConfigureAcronym("eps_estimate_current_year", "EPSEstimateCurrentYear")
    ConfigureAcronym("eps_estimate_next_year", "EPSEstimateNextYear")
    ConfigureAcronym("eps_estimate_next_quarter", "EPSNextQuarter")
    ConfigureAcronym("eps_estimate_current_quarter", "EPSEstimateCurrentQuarter")
    ConfigureAcronym("ebitda", "EBITDA")
    ConfigureAcronym("operating_margin_ttm", "OperatingMarginTTM")
    ConfigureAcronym("return_on_assets_ttm", "ReturnOnAssetsTTM")
    ConfigureAcronym("return_on_equity_ttm", "ReturnOnEquityTTM")
    ConfigureAcronym("revenue_ttm", "RevenueTTM")
    ConfigureAcronym("revenue_per_share_ttm", "RevenuePerShareTTM")
    ConfigureAcronym("quarterly_revenue_growth_yoy", "QuarterlyRevenueGrowthYOY")
    ConfigureAcronym("gross_profit_ttm", "GrossProfitTTM")
    ConfigureAcronym("diluted_eps_ttm", "DilutedEpsTTM")
    ConfigureAcronym("quarterly_earnings_growth_yoy", "QuarterlyEarningsGrowthYOY")
    ConfigureAcronym("two_hundred_day_ma", "TwoHundredDayMA")
    ConfigureAcronym("trailing_pe", "TrailingPE")
    ConfigureAcronym("forward_pe", "ForwardPE")
    ConfigureAcronym("price_sales_ttm", "PriceSalesTTM")
    ConfigureAcronym("price_book_mrq", "PriceBookMRQ")
    s = ToCamel(d)
    return
}

If you run this test and send the output to a file the panic is reproducible. If you do not output to a file then the standard output to the console actually slows down the process enough that the issue occurs much less frequently.

go test -run TestConcurrency > deleteme.txt

Will produce:

fatal error: concurrent map writes

goroutine 35 [running]:
runtime.throw({0x5184a7, 0x0})
        /usr/lib/go-1.17/src/runtime/panic.go:1198 +0x71 fp=0xc000146ec8 sp=0xc000146e98 pc=0x433ef1
runtime.mapassign_faststr(0x0, 0x0, {0x516826, 0xe})
        /usr/lib/go-1.17/src/runtime/map_faststr.go:211 +0x39c fp=0xc000146f30 sp=0xc000146ec8 pc=0x411e9c
github.com/iancoleman/strcase.ConfigureAcronym(...)
        /home/heath/src/strcase/acronyms.go:11
github.com/iancoleman/strcase.convertDatabaseNameToCamelCase({0x514840, 0x4})
        /home/heath/src/strcase/concurrency_test.go:21 +0x45 fp=0xc000146f60 sp=0xc000146f30 pc=0x4e6245
github.com/iancoleman/strcase.doConvert()
        /home/heath/src/strcase/concurrency_test.go:15 +0xc8 fp=0xc000146fe0 sp=0xc000146f60 pc=0x4e61c8
runtime.goexit()
        /usr/lib/go-1.17/src/runtime/asm_amd64.s:1581 +0x1 fp=0xc000146fe8 sp=0xc000146fe0 pc=0x463501
created by github.com/iancoleman/strcase.TestConcurrency
        /home/heath/src/strcase/concurrency_test.go:7 +0x2b

The fix:

Change acronyms.go from:

var uppercaseAcronym = map[string]string{
    "ID": "id",
}

// ConfigureAcronym allows you to add additional words which will be considered acronyms
func ConfigureAcronym(key, val string) {
    uppercaseAcronym[key] = val
}

To:

var uppercaseAcronym = sync.Map{}

// ConfigureAcronym allows you to add additional words which will be considered acronyms
func ConfigureAcronym(key, val string) {
    uppercaseAcronym.Store(key, val)
}

Change camel.go toCamelInitCase lines 39-41 from:

if a, ok := uppercaseAcronym[s]; ok {
    s = a
}

To:

if a, ok := uppercaseAcronym.Load(s); ok {
    s, _ = a.(string)
}

The you need to add the following to camel_test.go

ConfigureAcronym("ID", "Id")

I have a version of this fix as a branch if you want it.

mamyn0va commented 1 year ago

I have the same issue, a data race between acronyms.go and camel.go. I can provide logs if needed.

mamyn0va commented 1 year ago

@iancoleman would it be ok for you to have a PR with the fix of @Snookmz ?

iancoleman commented 1 year ago

Thanks for this!