Repository navigation
fix(uaparser): clone the shared default RegexDefinitions in New - #102
Conversation
The cached default returned by defaultRegexesDefinitions is a sync.OnceValue of a RegexDefinitions value, but its UA/OS/Device fields are slices of pointers shared across every New call. mustCompile then writes p.Reg on those shared pointers, racing whenever two parsers initialize concurrently (seen via statsig-io/go-sdk spawning a parser per client). Closes ua-parser#100 Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
dgoldstein0
left a comment
dgoldstein0
left a comment
There was a problem hiding this comment.
assuming this passes tests the fix looks good. Took me a moment to determine there's not an extra copy of regexesDefinitions as .Clone() takes and returns pointers.
any chance of wiring up -race to our CI? https://github.com/ua-parser/uap-go/blob/0623ea3aeb18d27815c335a34069938c7c1ffa17/.github/workflows/ci.yml - not sure if it's best as a separate step or as part of the existing "Run tests". If you want that can be a separate PR, or just include in this one.
| // TestNewConcurrent_NoRaceOnDefaultRegexDefinitions reproduces the data race | ||
| // reported in issue #100. Without the fix, multiple goroutines calling New() | ||
| // with the default cached RegexDefinitions racing on the shared []*uaParser | ||
| // pointers triggers a race detector failure under -race. |
There was a problem hiding this comment.
hm, I think our CI doesn't run with -race - perhaps that could be easily added? That would guarantee that we stay race-free much better than relying on contributors to manually run tests with -race. Any chance of adding that to this PR?
|
... well I suppose a fix is better than nothing, so I'm going to merge as is and file a task for adding a -race version to our CI - feel free to contribute the PR for that. |
|
filed that followup as #104 |
2 participants
The cached default returned by
defaultRegexesDefinitionsis async.OnceValueof aRegexDefinitionsvalue, but its UA/OS/Device fields are slices of pointers shared across everyNewcall, somustCompilewritesp.Regon the same elements from two goroutines initializing in parallel. Cloning the cached default inNewgives each parser its own pointer set, and a new-racetest exercises 16 concurrentNew()calls.Closes #100