Skip to content

[WIP] Fix data race caused by blevesearch/bleve #5217

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

Closed
wants to merge 2 commits into from

Conversation

typeless
Copy link
Contributor

For #5189

@typeless typeless force-pushed the fix-5189-v1 branch 3 times, most recently from 98881cc to e424f4a Compare October 29, 2018 15:09
@lunny
Copy link
Member

lunny commented Oct 29, 2018

So many files changed and it's difficult to review.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 29, 2018
@typeless
Copy link
Contributor Author

@lunny This PR has not been finished yet.

@lunny
Copy link
Member

lunny commented Oct 30, 2018

@typeless see #5224, that will fix getOwnerName data race.

@typeless
Copy link
Contributor Author

I based the PR on the master of blevesearch/bleve. Are we going to upgrade to the latest bleve?
If not, I would have to rebase this on the vendored version, which would also reduce the size of diffs.

But how will it look like if I make changes on a vendored package? AFAIK, the Makefile would reject any local change to the vendor directory.

@typeless
Copy link
Contributor Author

Fixing it upstream is a bit of cumbersome, it would be great if I can fix it in Gitea alone like #5224

@lunny
Copy link
Member

lunny commented Oct 30, 2018

@typeless update the vendor could be a standalone PR. Is this data race made by https://github.com/blevesearch/bleve, https://github.com/ethantkoenig/rupture or gitea's code?

@typeless
Copy link
Contributor Author

@lunny See https://github.com/ethantkoenig/rupture/blob/master/flushing_batch.go#L60-L67

The method Batch would spawn a goroutine which reads the pointer fields of b.batch.

Meanwhile, the following Reset method call of b.batch would update the value of the pointers, which results in the data race.

I can not prove if the map entries (other than the value of the map itself) of b.batch are safe or not. So, I added sync.Map for it also, which might not be a good idea.

Because the goroutine is spawned inside the bleve code, it's hard to predict the behavior from upper layers.

@lunny
Copy link
Member

lunny commented Dec 19, 2018

Would you like to continue this PR or maybe we could close this one.

@typeless
Copy link
Contributor Author

Got it. Won't have time to work on this in the near future.

@typeless typeless closed this Dec 19, 2018
@typeless typeless deleted the fix-5189-v1 branch April 3, 2019 05:00
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants