Skip to content

Commit 05b55b0

Browse files
authored
Fix data race with AbstractPageMappingToIteratorOperator (elastic#130963)
We need to release the blocks of the page in AbstractPageMappingToIteratorOperator immediately in single-iteration cases, instead of delaying to the next iteration. This is because the blocks of the page are now shared with the output page. The output page can be passed to a separate driver, which may run concurrently with this driver, leading to data races in AbstractNonThreadSafeRefCounted, which is not thread-safe. An alternative would be to make RefCounted for Vectors/Blocks thread-safe when they are about to be shared with other drivers via #allowPassingToDifferentDriver. Relates elastic#130573 Closes elastic#130959 Closes elastic#130958 Closes elastic#130950 Closes elastic#130925 Closes elastic#130881 Closes elastic#130796
1 parent 0a79b4c commit 05b55b0

File tree

2 files changed

+14
-5
lines changed

2 files changed

+14
-5
lines changed

muted-tests.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -555,9 +555,6 @@ tests:
555555
- class: org.elasticsearch.xpack.esql.action.EsqlRemoteErrorWrapIT
556556
method: testThatRemoteErrorsAreWrapped
557557
issue: https://github.com/elastic/elasticsearch/issues/130794
558-
- class: org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT
559-
method: test {ints.InCast SYNC}
560-
issue: https://github.com/elastic/elasticsearch/issues/130796
561558
- class: org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT
562559
method: test {p0=mtermvectors/10_basic/Tests catching other exceptions per item}
563560
issue: https://github.com/elastic/elasticsearch/issues/122414

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/AbstractPageMappingToIteratorOperator.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,7 @@ public TransportVersion getMinimalSupportedVersion() {
321321
private static class AppendBlocksIterator implements ReleasableIterator<Page> {
322322
private final Page page;
323323
private final ReleasableIterator<Block[]> next;
324+
private boolean closed = false;
324325

325326
private int positionOffset;
326327

@@ -348,7 +349,15 @@ public final Page next() {
348349
for (int b = 0; b < page.getBlockCount(); b++) {
349350
page.getBlock(b).incRef();
350351
}
351-
return page.appendBlocks(read);
352+
final Page result = page.appendBlocks(read);
353+
// We need to release the blocks of the page in this iteration instead of delaying to the next,
354+
// because the blocks of this page are now shared with the output page. The output page can be
355+
// passed to a separate driver, which may run concurrently with this driver, leading to data races
356+
// of references in AbstractNonThreadSafeRefCounted, which is not thread-safe.
357+
// An alternative would be to make RefCounted for Vectors/Blocks thread-safe when they are about
358+
// to be shared with other drivers via #allowPassingToDifferentDriver.
359+
close();
360+
return result;
352361
}
353362
Block[] newBlocks = new Block[page.getBlockCount() + read.length];
354363
System.arraycopy(read, 0, newBlocks, page.getBlockCount(), read.length);
@@ -368,7 +377,10 @@ public final Page next() {
368377

369378
@Override
370379
public void close() {
371-
Releasables.closeExpectNoException(page::releaseBlocks, next);
380+
if (closed == false) {
381+
closed = true;
382+
Releasables.closeExpectNoException(page::releaseBlocks, next);
383+
}
372384
}
373385
}
374386
}

0 commit comments

Comments
 (0)