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

Fix getRouteHash concurrency safety #160

Merged
merged 4 commits into from
Jun 3, 2022
Merged

Conversation

wnxd
Copy link
Contributor

@wnxd wnxd commented Jun 3, 2022

No description provided.

@wnxd
Copy link
Contributor Author

wnxd commented Jun 3, 2022

Since there is no lock for map reading and writing, panic seems to occur under concurrency.

Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd be happier to not use sync.Map with casting
instead use a sync.Mutex

@wnxd
Copy link
Contributor Author

wnxd commented Jun 3, 2022

I think I'd be happier to not use sync.Map with casting instead use a sync.Mutex

Emmmm, I changed it to sync.Mutex.

@wnxd wnxd requested a review from topi314 June 3, 2022 11:48
Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides that small thingy looks good to me

rest/rest_rate_limiter_impl.go Show resolved Hide resolved
remove `l.hashesMu = sync.Mutex{}`

Co-authored-by: ToπSenpai <15636011+TopiSenpai@users.noreply.github.com>
@wnxd
Copy link
Contributor Author

wnxd commented Jun 3, 2022

besides that small thingy looks good to me

I saw that l.bucketsMu = sync.Mutex{} was used above. So I added l.hashes = sync.Mutex{}

@topi314
Copy link
Member

topi314 commented Jun 3, 2022

ohh nevermind it should be there yes

I thought this was in the constructor lol

@wnxd
Copy link
Contributor Author

wnxd commented Jun 3, 2022

Ok. I add it.

@wnxd wnxd requested a review from topi314 June 3, 2022 12:13
@topi314 topi314 merged commit b385c99 into disgoorg:development Jun 3, 2022
@topi314
Copy link
Member

topi314 commented Jun 3, 2022

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants