Skip to content

Commit 3d3dbbc

Browse files
committed
Fix data race with AbstractPageMappingToIteratorOperator
1 parent 3a5c74f commit 3d3dbbc

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)