chore: add generic SyncMap[K, V] and migrate sync.Map usages#7124
Open
chore: add generic SyncMap[K, V] and migrate sync.Map usages#7124
Conversation
spboyer
requested review from
hemarina,
vhvb1989,
wbreza and
weikanglim
as code owners
spboyer
requested review from
JeffreyCA,
rajeshkamal5050 and
tg-msft
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a generic, typed pkg/syncmap.Map[K,V] wrapper around sync.Map and migrates existing sync.Map usages in cli/azd (plus related tests) to use the typed API, reducing type assertions at call sites.
Changes:
- Introduce
pkg/syncmap.Map[K,V]as a type-safe wrapper oversync.Map. - Migrate
sync.Mapfields/locals across multiple packages (grpcbroker, auth, containerapps, devcentersdk, ux, ai, internal/cmd/add). - Update grpcbroker tests to reflect typed map return values and typed
Rangecallbacks.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/ux/canvas.go | Migrates canvas manager’s internal registry from sync.Map to syncmap.Map. |
| cli/azd/pkg/syncmap/syncmap.go | Adds new generic typed wrapper around sync.Map. |
| cli/azd/pkg/grpcbroker/message_broker_test.go | Updates tests to remove now-unnecessary type assertions and use typed Range. |
| cli/azd/pkg/grpcbroker/message_broker.go | Replaces broker sync.Map usage with typed maps and simplifies channel/handler retrieval. |
| cli/azd/pkg/devcentersdk/developer_client.go | Splits the prior generic cache into typed project/devcenter caches using syncmap.Map. |
| cli/azd/pkg/containerapps/container_app.go | Migrates ARM client caches from sync.Map to typed syncmap.Map. |
| cli/azd/pkg/auth/credential_providers.go | Migrates tenant credential cache from sync.Map to typed syncmap.Map. |
| cli/azd/pkg/ai/model_service.go | Uses typed syncmap.Map for concurrent collection of per-location quota results. |
| cli/azd/internal/cmd/add/add_select_ai.go | Uses typed syncmap.Map for concurrent model catalog aggregation across locations. |
Comments suppressed due to low confidence (1)
cli/azd/pkg/grpcbroker/message_broker.go:679
- Close() closes all response channels while processMessage goroutines may still be running (Run() spawns them with
go mb.processMessage(...)and does not wait). A concurrent send to a channel that Close() has closed will panic. Consider tracking processMessage goroutines with a WaitGroup and waiting before closing channels, or avoiding closing channels from Close() and instead signaling waiters via context/done channel to prevent send-on-closed-channel races.
// Close gracefully shuts down the broker (optional, for cleanup)
func (mb *MessageBroker[TMessage]) Close() {
// Close all pending channels
mb.responseChans.Range(func(key string, ch chan *TMessage) bool {
close(ch)
mb.responseChans.Delete(key)
return true
})
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+13
to
+22
| // Load returns the stored value for key. | ||
| func (s *Map[K, V]) Load(key K) (V, bool) { | ||
| v, ok := s.m.Load(key) | ||
| if !ok { | ||
| var zero V | ||
| return zero, false | ||
| } | ||
|
|
||
| return v.(V), true | ||
| } |
Comment on lines
+7
to
+9
|
|
||
| // Map is a type-safe wrapper around sync.Map that eliminates type assertions at call sites. | ||
| type Map[K comparable, V any] struct { |
Comment on lines
501
to
511
| if ch, ok := mb.responseChans.Load(requestId); ok { | ||
| channelTyped := ch.(chan *TMessage) | ||
|
|
||
| // Check if channel is full | ||
| if len(channelTyped) >= cap(channelTyped)-1 { | ||
| if len(ch) >= cap(ch)-1 { | ||
| mb.logger.Printf( | ||
| "[%s] WARNING: Channel buffer nearly full for RequestId=%s (len=%d, cap=%d)", | ||
| mb.name, | ||
| requestId, | ||
| len(channelTyped), | ||
| cap(channelTyped), | ||
| len(ch), | ||
| cap(ch), | ||
| ) | ||
| } |
Create a type-safe syncmap.Map[K, V] wrapper that eliminates type assertions at every Load/Store/Range call site. Migrate all existing sync.Map usages to the typed wrapper. Fixes #7089 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
spboyer
force-pushed
the
feat/syncmap-generic
branch
from
683cb9b to
94e80bd
Compare
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- add a generic pkg/syncmap.Map[K, V] wrapper around sync.Map\n- migrate all non-test sync.Map usages in cli/azd to typed syncmap.Map fields/locals\n- update grpcbroker tests for the typed map API\n\n## Testing\n- go build ./...\n- go test ./pkg/syncmap/... -count=1\n- go test ./pkg/grpcbroker/... ./pkg/auth/... ./pkg/devcentersdk/... ./pkg/ux/... ./pkg/ai/... ./pkg/containerapps/... ./internal/cmd/... -count=1 -short