-
Notifications
You must be signed in to change notification settings - Fork 24
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
IOOBE in TransportUpdateByQueryAction$MultipleIndexUpdateByQueryActionListener.onResponse()
#22
Comments
In TransportUpdateByQueryAction.MultipleIndexUpdateByQueryActionListener.onResponse(), we can read: public void onResponse(IndexUpdateByQueryResponse indexUpdateByQueryResponse) {
successFullIndexResponses.set(indexCounter.getAndIncrement(), indexUpdateByQueryResponse);
if (indexCounter.get() == expectedNumberOfResponses) {
finishHim();
}
}
public void onFailure(Throwable e) {
failedIndexResponses.set(indexCounter.getAndIncrement(), e);
if (indexCounter.get() == expectedNumberOfResponses) {
finishHim();
}
} The IOOBE happened in the What happened is that somehow void handleResponse(ShardUpdateByQueryResponse response) {
shardResponses.set(indexCounter.getAndIncrement(), response);
if (indexCounter.get() == numberOfExpectedShardResponses) {
finalizeAction();
}
}
void handleException(Throwable e, ShardRouting shard) {
logger.error("[{}][{}] error while executing update by query shard request", e, request.index(), shard.id());
String failure = ExceptionsHelper.detailedMessage(e);
shardResponses.set(indexCounter.getAndIncrement(), new ShardUpdateByQueryResponse(shard.id(), failure));
if (indexCounter.get() == numberOfExpectedShardResponses) {
finalizeAction();
}
} |
There is a racing condition with the following listener idiom: final int numberOfExpectedShardResponses;
final AtomicInteger indexCounter = new AtomicInteger();
void noteResponse(Response response) {
responses.set(indexCounter.getAndIncrement(), response);
if (indexCounter.get() == numberOfExpectedResponses) {
finalizeAction();
}
} Suppose the following interleaving of operations on two threads:
This exhibits 2 problems:
A possible fix would be to use another AtomicInteger, incremented and compared to the number of expected responses only after the response is set in the array: final int numberOfExpectedShardResponses;
final AtomicInteger indexCounter = new AtomicInteger();
final AtomicInteger writeCounter = new AtomicInteger();
void noteResponse(Response response) {
responses.set(indexCounter.getAndIncrement(), response);
if (writeCounter.incrementAndGet() == numberOfExpectedResponses) {
finalizeAction();
}
} This way, every single index from indexCounter is still used by only one thread at a time, like before, but now only one thread will have the responsibility to finalize, and it could only do so after all values have been properly written. |
@martijnvg Shouldn't The initial problem may not be directly related to this one, as I still can't explain why |
Maybe it was just a race condition in |
@ofavre Those base classes are actually meant to perform a write and replica it. The |
I got a strange error while running the tests multiple time with the same seed.
I was unable to reproduce using the command suggested in the output.
I was working on another bug due to / hinted by randomization, in which I only modified the number of shards inside in the settings of an client().admin().indices().prepareCreate() call. It should not interfere anyway.
I was on commit 349d105.
The text was updated successfully, but these errors were encountered: