fix: copy-on-write in updateUserInfo to avoid shared-map data race#317
fix: copy-on-write in updateUserInfo to avoid shared-map data race#317ThiagoBauken wants to merge 2 commits into
Conversation
updateUserInfo mutated values.(Values).m[field] in place. That map is shared: it lives in userinfocache and is handed to request goroutines via the request context, so an in-place write races with concurrent readers (Values.Get) and can crash the process with "concurrent map read and map write". Build a fresh map and return a new Values instead. All callers already persist the returned value via userinfocache.Set, so behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements a copy-on-write pattern in updateUserInfo to prevent concurrent map read and write data races, and adds unit tests to verify its correctness and concurrent safety. Feedback on the changes points out a potential runtime panic due to a direct type assertion on values, suggesting the use of a comma-ok assertion to handle nil or unexpected types safely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // and can crash the process with "concurrent map read and map write". | ||
| // Build a fresh map and return a new Values; callers persist it via | ||
| // userinfocache.Set. | ||
| old := values.(Values) |
There was a problem hiding this comment.
Directly type-asserting values.(Values) will panic if values is nil (which can happen if the user info is missing or not yet set in the context/cache) or if it is of an unexpected type. Using the comma-ok type assertion form prevents runtime panics and safely falls back to the zero-value of Values (where m is nil), which is then safely handled by the rest of the function.
| old := values.(Values) | |
| old, _ := values.(Values) |
values.(Values) would panic on a nil or unexpected value. Use the comma-ok form so it falls back to the zero Values (nil map), which the copy loop handles safely. Addresses Gemini Code Assist review on asternic#317. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Closing to consolidate my recent PRs into a smaller, focused set that's easier to review — I'll resubmit the changes grouped together. Sorry for the churn, and thanks! |
Problem
updateUserInfomutates the map insideValuesin place:That map is shared: it is stored in
userinfocacheand handed to request goroutines through the request context (r.Context().Value("userinfo")). An in-place write therefore races with concurrent readers (Values.Get, called on every authenticated request by the access logger and throughout the handlers). Under load this is a classicfatal error: concurrent map read and map write, which crashes the whole process.Fix
Make
updateUserInfocopy-on-write: build a fresh map, copy the existing entries, set the field, and return a newValues. The shared map is never written. All call sites already use the returned value and persist it viauserinfocache.Set, so behaviour is unchanged.Testing
TestUpdateUserInfoCopyOnWrite— deterministic: asserts the sourceValuesis untouched and the returned one carries the update (plus unrelated fields). Verified red→green (fails against the in-place version, passes with the fix).TestUpdateUserInfoConcurrent— hammersupdateUserInfo+Getfrom many goroutines; undergo test -racethe old version reports the data race, the new one is clean.go build ./...✅ ·go vet ./...✅ ·go test ./...✅