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

fixes race condition in Searcher #1464

Merged
merged 3 commits into from
Aug 24, 2022
Merged

fixes race condition in Searcher #1464

merged 3 commits into from
Aug 24, 2022

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented 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

@fulmicoton
Copy link
Collaborator

@shikhar can you review #1464

fixes #1461
fixes a race condition in Searcher, by avoiding repeated calls to open_segment_readers and passing them instead as argument
src/core/searcher.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2022

Codecov Report

Merging #1464 (227c252) into main (67d94f5) will decrease coverage by 0.00%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
- Coverage   94.07%   94.06%   -0.01%     
==========================================
  Files         238      239       +1     
  Lines       44564    44636      +72     
==========================================
+ Hits        41922    41989      +67     
- Misses       2642     2647       +5     
Impacted Files Coverage Δ
src/core/searcher.rs 77.93% <85.71%> (+0.39%) ⬆️
src/reader/mod.rs 92.94% <100.00%> (-0.05%) ⬇️
src/fastfield/serializer/mod.rs 90.47% <0.00%> (-1.09%) ⬇️
src/fastfield/writer.rs 90.80% <0.00%> (-0.29%) ⬇️
fastfield_codecs/src/lib.rs 96.87% <0.00%> (-0.03%) ⬇️
src/query/mod.rs 100.00% <0.00%> (ø)
src/query/query.rs 85.71% <0.00%> (ø)
src/query/boost_query.rs 69.76% <0.00%> (ø)
fastfield_codecs/src/bitpacked.rs 100.00% <0.00%> (ø)
src/query/disjunction_max_query.rs 0.00% <0.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@shikhar shikhar left a comment

Choose a reason for hiding this comment

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

@PSeitz aside from clippy nits, LGTM

I did have one unrelated change I was sneaking into my PR, that I wanted to ask about.

Is Relaxed memory order safe here? Both a load and store are happening, so AcqRel seems safer to me. And this is by no means a hot path so being conservative seems fair.

let generation_id = searcher_generation_counter.fetch_add(1, atomic::Ordering::Relaxed);

@@ -204,7 +201,7 @@ impl InnerIndexReader {
Ok(segment_readers)
}

fn create_new_searcher_generation(
fn track_segment_readers_in_inventory(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous name seems better to me, since we are creating a SearcherGeneration and making sure to do it in a way that we track it int the inventory, and the inventory tracks SearcherGeneration not segment readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my issue with the previous name was that it's not clear that inventory is used to track and not the source of the generation

@PSeitz
Copy link
Contributor Author

PSeitz commented Aug 24, 2022

@PSeitz aside from clippy nits, LGTM

I did have one unrelated change I was sneaking into my PR, that I wanted to ask about.

Is Relaxed memory order safe here? Both a load and store are happening, so AcqRel seems safer to me. And this is by no means a hot path so being conservative seems fair.

let generation_id = searcher_generation_counter.fetch_add(1, atomic::Ordering::Relaxed);

Yeah, I think there could be scenarios where simultaneous reloads are triggered at the same time from multiple threads.

@fulmicoton
Copy link
Collaborator

fulmicoton commented Aug 24, 2022

@shikhar The operation is still Atomic... I think we just want to ensure that the id delivered are unique, and Relaxed is sufficient for that. AcqRel stuff is about instruction reordering.

@PSeitz
Copy link
Contributor Author

PSeitz commented Aug 24, 2022

@shikhar I think we just want to ensure that the id delivered are unique, and Relaxed is sufficient for that... That being said it is not a hot part of the code. We can use SeqCst and not sweat it.

I think it could be an issue when two threads increment, but due to weak fencing, we get two times the same value back. If now one of those callers has a new segment in their list, we have the same id to different SearcherGeneration.

It's very unlikely though, and on many platforms Relaxed is already AcqRel.

@adamreichold
Copy link
Collaborator

adamreichold commented Aug 24, 2022

I think it could be an issue when two threads increment, but due to weak fencing, we get two times the same value back.

The memory orderings mostly determine how other memory accesses are ordered w.r.t. the atomic accesses, they do not affect the atomicity of an operation like fetch_add, i.e. multiple threads doing fetch_add(1, Ordering::Relaxed) will never see identical values (ignoring overflow).

However, their accesses to other memory locations that are shared with other threads will not be ordered w.r.t. the ID generation, e.g. their accesses to searcher_generation_inventory are do not necessarily reflect their accesses to searcher_generation_counter, meaning that a thread with lower ID might see the stores to the inventory of a thread with a higher ID.

While I suspect that this is not an issue (this would have involve unsafe code if I understand things correctly), my personal choice would be Ordering::AcqRel as well just to capture any implicit dependencies which I do not see ATM. (I would suggest avoiding Ordering::SeqCst as mixing those two orderings can have unexpected effects and sticking to AcqRel is often preferable if global ordering is not required as it is simpler to reason about the explicit pairings between acquire and release operations to the same memory address.)

@fulmicoton
Copy link
Collaborator

fulmicoton commented Aug 24, 2022

It's very unlikely though, and on many platforms Relaxed is already AcqRel.

This is a very bad argument :). Also ARM is pretty popular.

we get two times the same value back

You cannot get two times the same value back... That's not what ordering is about.
The explanation here is ok:
https://en.cppreference.com/w/cpp/atomic/memory_order

...

Anyway let's defensively go for Ordering::AcqRel...

@fulmicoton fulmicoton merged commit bb01e99 into main Aug 24, 2022
@fulmicoton fulmicoton deleted the searcher_race_condition branch August 24, 2022 12:17
@shikhar
Copy link
Collaborator

shikhar commented Aug 24, 2022

Thanks folks, I need to learn more about memory orderings :)

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

Successfully merging this pull request may close these issues.

Searcher and its SearcherGeneration may reference a different set of segments
5 participants