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

Searcher and its SearcherGeneration may reference a different set of segments #1461

Closed
shikhar opened this issue Aug 22, 2022 · 2 comments · Fixed by #1464
Closed

Searcher and its SearcherGeneration may reference a different set of segments #1461

shikhar opened this issue Aug 22, 2022 · 2 comments · Fixed by #1464
Assignees
Labels

Comments

@shikhar
Copy link
Collaborator

shikhar commented Aug 22, 2022

Bug

With tantivy @ d24f31f,

ran into a situation where Gen 1 below of the Searcher has an inconsistent set of segment IDs referenced in its SearcherGeneration object.

Gen 0: good

Searcher(
	[
		Seg("2b9d1870"),
		Seg("de008a03"),
		Seg("1443006d"),
		Seg("ce5a247e"),
		Seg("70af8500"),
		Seg("7c542ba6")
	]
)

SearcherGeneration {
	segments: {
		Seg("1443006d"): Some(103364896),
		Seg("2b9d1870"): Some(103364896),
		Seg("70af8500"): Some(103364896),
		Seg("7c542ba6"): None,
		Seg("ce5a247e"): Some(103364896),
		Seg("de008a03"): Some(103364896),
	},
	generation_id: 0
}

Gen 1: bad

Searcher(
	[
		Seg("2b9d1870"),
		Seg("de008a03"),
		Seg("1443006d"),
		Seg("ce5a247e"),
		Seg("70af8500"),
		Seg("68356e6c”),
	]
)

SearcherGeneration {
	segments: {
		Seg("1240c4ed"): None,
		Seg("1443006d"): Some(103367743),
		Seg("2b9d1870"): Some(103367743),
		Seg("5a475796"): None,
		Seg("7c542ba6"): Some(103367743),
		Seg("70af8500"): Some(103367743),
		Seg("ce5a247e"): Some(103367743),
		Seg("de008a03"): Some(103367743)
	},
	generation_id: 1,
}

Reason

Looks like this may have been a regression introduced in 23fe73a cc @PSeitz

In between the below calls to open_segment_readers() is potential for a race condition.

let segment_readers = Self::open_segment_readers(&self.index)?;

let segment_readers = Self::open_segment_readers(index)?;

The solution seems to be to only call it once when creating a Searcher, this used to be the case before. And maybe we can add an assertion that the set of SegmentIds is identical.

@shikhar shikhar self-assigned this Aug 22, 2022
@fulmicoton fulmicoton added the bug label Aug 22, 2022
PSeitz added a commit that referenced this issue Aug 23, 2022
fixes #1461
fixes a race condition in Searcher, by avoiding repeated calls to open_segment_readers and passing them instead as argument
@PSeitz
Copy link
Contributor

PSeitz commented Aug 23, 2022

@shikhar Thanks for the report, I think your analysis is correct. I created a PR, which is a little more minimal than yours, by passing the segment_readers as argument instead of reloading.

Did you experience a crash and if yes can you post the error message so it's more discoverable if someone else has the same problem?

PSeitz added a commit that referenced this issue Aug 23, 2022
fixes #1461
fixes a race condition in Searcher, by avoiding repeated calls to open_segment_readers and passing them instead as argument
@shikhar
Copy link
Collaborator Author

shikhar commented Aug 23, 2022

Did you experience a crash and if yes can you post the error message so it's more discoverable if someone else has the same problem?

@PSeitz this was from a panic in our custom Query, no Tantivy-level error to share :-) It was expecting some segment-level state that is warmed at the SegmentId level and did not find it.

Warmer::warm() is provided Searcher, and Warmer::garbage_collect() is provided SearcherGenerations, so this was leading to a segment's state being warmed and then garbage-collected while the Searcher was still around.

fulmicoton pushed a commit that referenced this issue Aug 24, 2022
Fixes a race condition in Searcher, by avoiding repeated calls to open_segment_readers and passing them instead as argument

Closes #1461
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants