Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix gomap datarace #90

Merged
merged 2 commits into from
Feb 10, 2020
Merged

Fix gomap datarace #90

merged 2 commits into from
Feb 10, 2020

Conversation

tdakkota
Copy link
Contributor

@tdakkota tdakkota commented Feb 8, 2020

delete is not thread safe and must be synchronized

@tdakkota tdakkota requested a review from philippgille February 9, 2020 13:02
@tdakkota tdakkota changed the base branch from master to develop February 10, 2020 11:58
@philippgille
Copy link
Owner

Hi @tdakkota, thank you so much for the 4 PRs!

I'm currently short on time, but this one is really important and it's just a small change, so I had a look. Totally makes sense, not sure how I missed that! It's great that you already changed the target branch to develop, so despite the Consul tests failing (https://travis-ci.org/philippgille/gokv/jobs/647844727#L2464) I'll merge this right now and have a look at the Consul tests as soon as possible, maybe this weekend.

I'll also have a look at the other PRs as soon as possible. Thanks a lot already, it's great to know others like the idea to have many implementations for a simple interface as well!

@philippgille philippgille merged commit d421395 into philippgille:develop Feb 10, 2020
@tdakkota
Copy link
Contributor Author

Hi @philippgille!
When I changed

https://github.com/philippgille/gokv/blob/master/consul/consul_test.go#L69

goroutineCount := 1000

to

goroutineCount := 10

it passed.

I think it is Consul bug

@tdakkota tdakkota deleted the fix-gomap branch February 23, 2020 22:53
philippgille added a commit that referenced this pull request May 7, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants