Skip to content

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