Skip to content

Commit e60efe2

Browse files
authored
Fix data race with AbstractPageMappingToIteratorOperator (#130963) (#130972)
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 Relates #130573 Closes #130959 Closes #130958 Closes #130950 Closes #130925 Closes #130881 Closes #130796
1 parent 79cc270 commit e60efe2

File tree

1 file changed

+14
-2
lines changed

1 file changed

+14
-2
lines changed

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)