Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Struct Group is not goroutine safe #296

Open
ghost opened this issue Apr 25, 2022 · 8 comments
Open

Struct Group is not goroutine safe #296

ghost opened this issue Apr 25, 2022 · 8 comments
Labels
bug Something isn't working no-stalebot

Comments

@ghost
Copy link

ghost commented Apr 25, 2022

Describe the bug?

Hi, trying to pass Group to several goroutines, which some of them try to Marshal it, results in fatal error: concurrent map writes. It seems like the cause is this, under GroupProfile.MarshalJSON():

if a.Name != "" {
		a.GroupProfileMap["name"] = a.Name
}

image

Thanks!

What is expected to happen?

Safely marshal a struct in parallel

What is the actual behavior?

Transient fatal error: concurrent map writes

Reproduction Steps?

Just marshal in parallel until it happens

Additional Information?

No response

Golang Version

go version go1.18.1 darwin/amd64

SDK Version

v2.12.1

OS version

No response

@ghost ghost added the bug Something isn't working label Apr 25, 2022
@monde monde self-assigned this May 3, 2022
@monde
Copy link
Collaborator

monde commented May 3, 2022

Apologies @boazshalom , I missed this issue. I will look into it.

@ghost
Copy link
Author

ghost commented May 4, 2022

All good @monde, thank you!

@github-actions
Copy link

This issue has been marked stale because there has been no activity within the last 14 days. To keep this issue active, remove the stale label.

@github-actions github-actions bot added the stale label May 19, 2022
@monde monde added no-stalebot and removed stale labels May 19, 2022
@ghost
Copy link
Author

ghost commented Jul 24, 2022

Hi @monde, any update please?

@monde
Copy link
Collaborator

monde commented Jul 25, 2022

I'll try to carve out some time for this @boazshalom , thanks for the ping.

@monde
Copy link
Collaborator

monde commented Jul 26, 2022

@boazshalom I started looking at this and working on a PR. Is your code writing to GroupProfileMap as well? I'm not seeing how the existing implementation in MarshalJSON() would be causing this unless you were making many concurrent API calls with the same group. Seems like you wouldn't want to be calling the API like that, but I'd have to see your implementation to improve my impression. However, read on for history and an explanation of my PR:

We added the map named GroupProfileMap to GroupProfile per this feedback from @ymylei in #268 . GroupProfileMap is a true golang map, and as a developer/user of the okta-sdk-golang you'd have to write your own concurrency protection if you were writing to GroupProfileMap directly in your code.

If I were to refactor GroupProfileMap into a sync.Map it would look like the test example in PR #318 (e.g. profile.GroupProfileMap.Store(key, value) instead of profile.GroupProfileMap[key] = value. But then everyone would have a breaking change on the next minor release of the SDK.

https://github.com/okta/okta-sdk-golang/blob/718268404f7daf4b31a2e317e8332eed04f8e6a7/tests/integration/group_profile_test.go

All that said, perhaps I should just put a mutex on a.GroupProfileMap["name"] = a.Name and a.GroupProfileMap["description"] = a.Description in MarshalJSON() and then that would truly put the onus on anyone writing to GroupProfileMap in their implementation to also make use of a mutex.

Would like to get your feedback @boazshalom , hopefully @ymylei can give feedback also.

@monde
Copy link
Collaborator

monde commented Jul 27, 2022

@boazshalom @ymylei I think my #268 PR is over kill. I think I will put up a new PR with mutex around https://github.com/okta/okta-sdk-golang/blob/master/okta/groupProfile.go#L60-L65 and also leave a comment about developers making use of GroupProfiles's GroupProfilesMap needing to also put a mutex around their implementation.
cc: @duytiennguyen-okta

@ymylei
Copy link
Contributor

ymylei commented Aug 7, 2022

@monde Based on my reading of https://pkg.go.dev/sync#Map this does seem overkill for this data set. Particularly, I don't see how this data class in particular would need to be made go routine safe by itself, versus this is something which needs larger attention in the library (such as altering this for the UserProfile object as well).

@boazshalom I'd be curious if you'd be able to share the use case you're doing which causes you to send the group object into a set of go routines. While I do personally think this type of multi-threaded use behaviour is going to need to be developer managed, I am curious if there's something about the behaviour when we configure the map to merge into the rest of the GroupProfile object which is problematic.

@monde monde removed their assignment Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-stalebot
Projects
None yet
Development

No branches or pull requests

2 participants