change topicArns to map safe for concurrent access

[nenikola]

Description

Changed native map to xsync.MapOf which is safe for concurrent access

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #3430

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

[yaron2]

Thanks for this PR. Can you please replace the usage of the xsync package with Go's sync.Map? It has the same methods. https://pkg.go.dev/golang.org/x/sync/syncmap

[nenikola]

Thanks for this PR. Can you please replace the usage of the xsync package with Go's sync.Map? It has the same methods. https://pkg.go.dev/golang.org/x/sync/syncmap

@yaron2 sure, I opted for xsync just because I saw it being used in topicsLocker.

Also, I saw subscriptions and queues are also of type regular map and seems like both could potentially face similar concurrent write issue. Would it make sense to change those to sync.Map too?

[ItalyPaleAle]

In this file we were doing synchronization using a mutex. For example, calls to getOrCreateTopic are wrapped around a lock:

components-contrib/pubsub/aws/snssqs/snssqs.go

Lines 762 to 763 in 95a0a13

	s.topicsLocker.Lock(req.Topic)
	defer s.topicsLocker.Unlock(req.Topic)

Rather than changing topicArns, it may make more sense to just ensure that wherever the access to the map was happening that wasn't wrapped in a log (and was causing the panic), uses the lock instead

[nenikola]

@ItalyPaleAle so I did something similar to what is done in the Subscribe method by locking the topic when invoking getOrCreateTopic but would it make more sense for the method getOrCreateTopic to do the locking itself instead of its caller doing so?

The way it is now, caller of getOrCreateTopic needs to know that getOrCreateTopic method does unsafe map write, which shouldn't be the caller's concern per my understanding.

[ItalyPaleAle]

@nenikola thanks for making the change.

I think adding a requirement to have a lock before calling a method is not an unusual thing. It is especially helpful in some cases such as when the caller may invoke the method twice. However perhaps we could add a comment to getOrCreateTopic to indicate that locks should be acquired by callers?

[nenikola]

@ItalyPaleAle sounds good, let me add comment and then merge the changes. Thanks!

[ItalyPaleAle]

Thank you for this contribution and for fixing the issue!

[marcduiker]

@holopin-bot @nenikola Thank you Nikola!

[holopin-bot]

Congratulations @nenikola, the maintainer of this repository has issued you a badge! Here it is: https://holopin.io/claim/clzv9vq4q125210cjyqv675ud2

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!