-
Notifications
You must be signed in to change notification settings - Fork 628
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
ISPN-4626 Race condition in the LocalEntryRetriever between iterator() a... #2791
ISPN-4626 Race condition in the LocalEntryRetriever between iterator() a... #2791
Conversation
@@ -333,34 +333,33 @@ public Itr(int batchSize) { | |||
@Override | |||
public boolean hasNext() { | |||
boolean hasNext = !queue.isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer needs an initial value.
I believe this PR also solves ISPN-4594, right? |
Yes, and all the failures in the indexless queries tests which sometimes returns no results, not sure if they are all being tracked in Jiras |
I've removed the inital value of |
@@ -333,34 +333,33 @@ public Itr(int batchSize) { | |||
@Override | |||
public boolean hasNext() { | |||
boolean hasNext = !queue.isEmpty(); | |||
if (!hasNext && !completed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the exact case here? I think we are adding a lot of extra contention by always aquiring the lock. I think what was happening is the following which is different than what you stated.
thread 1 sees nothing in queue so hasNext was false
thread 1 is put to sleep
thread 2 adds values and then completes the iterator
thread 1 wakes up and sees completed as true
thread 1 then evaluates this if block and finds no changes.
I am thinking all the change that is required is to remove && !completed
. So this way it would force a recheck of the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is, in a foreach loop involving the entryRetriever, iterator() and hasNext() are called implicitly one after another. The iterator construction schedules a thread to start iterating through the data container entries, and sometimes it gets delayed, so a call to hasNext returns false and the client code fails, since the foreach loop is never executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I can see false being returned erroneously is the case I described unless you thought of something else? The sleep doesn't even have to occur either it could just be really good timing :)
PR udpate with @wburns suggestion: seems to solve the race condition with less contention |
Looks like this needs rebase and undoing the previous change. |
Rebased |
Pulling... |
Integrated into master, thanks @gustavonalle ! |
...nd hasNext()
https://issues.jboss.org/browse/ISPN-4626