Skip to content

Commit d4a7d2b

Browse files
committed
ESQL: Fix Driver creating status with a live list of operators (#132260)
`DriverStatus` is an immutable record created by the Driver. However, its components are not inherently immutable. This PR fixes a live collection used by the Driver, that was being directly put into the status, leading to `ConcurrentModificationException`s when reading it (Through the Task list API, for example). Some example errors: ``` java.util.ConcurrentModificationException at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1096) at java.base/java.util.ArrayList$Itr.next(ArrayList.java:1050) at org.elasticsearch.compute.operator.DriverStatus.toXContent(DriverStatus.java:134) at [email protected]/org.elasticsearch.xcontent.XContentBuilder.value(XContentBuilder.java:993) at [email protected]/org.elasticsearch.xcontent.XContentBuilder.field(XContentBuilder.java:978) at [email protected]/org.elasticsearch.tasks.TaskInfo.toXContent(TaskInfo.java:113) at [email protected]/org.elasticsearch.action.admin.cluster.node.tasks.list.TaskGroup.toXContent(TaskGroup.java:63) at [email protected]/org.elasticsearch.action.admin.cluster.node.tasks.list.TaskGroup.toXContent(TaskGroup.java:67) at [email protected]/org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse.lambda$groupedByParent$10(ListTasksResponse.java:183) ``` And: ``` java.util.ConcurrentModificationException at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1096) at java.base/java.util.ArrayList$Itr.next(ArrayList.java:1050) at org.elasticsearch.compute.operator.DriverStatus.documentsFound(DriverStatus.java:157) at org.elasticsearch.compute.operator.DriverStatus.toXContent(DriverStatus.java:129) at [email protected]/org.elasticsearch.xcontent.XContentBuilder.value(XContentBuilder.java:993) at [email protected]/org.elasticsearch.xcontent.XContentBuilder.field(XContentBuilder.java:978) at [email protected]/org.elasticsearch.tasks.TaskInfo.toXContent(TaskInfo.java:113) at org.elasticsearch.server@9.2.0org.elasticsearch.action.admin.cluster.node.tasks.list.TaskGroup.toXContent(TaskGroup.java:63) at [email protected]/org.elasticsearch.action.admin.cluster.node.tasks.list.TaskGroup.toXContent(TaskGroup.java:67) at [email protected]/org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse.lambda$groupedByParent$10(ListTasksResponse.java:183) ``` Also, this looks like the source of this issue, with another similar case: Fixes #131564
1 parent a1388b8 commit d4a7d2b

File tree

3 files changed

+61
-1
lines changed

3 files changed

+61
-1
lines changed

docs/changelog/132260.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 132260
2+
summary: FIx Driver creating status with a live list of operators
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 131564

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ private void updateStatus(long extraCpuNanos, int extraIterations, DriverStatus.
531531
prev.cpuNanos() + extraCpuNanos,
532532
prev.iterations() + extraIterations,
533533
status,
534-
statusOfCompletedOperators,
534+
List.copyOf(statusOfCompletedOperators),
535535
activeOperators.stream().map(op -> new DriverStatus.OperatorStatus(op.toString(), op.status())).toList(),
536536
sleeps
537537
);

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/operator/DriverTests.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@
5050

5151
import static org.hamcrest.Matchers.either;
5252
import static org.hamcrest.Matchers.equalTo;
53+
import static org.hamcrest.Matchers.not;
54+
import static org.hamcrest.Matchers.sameInstance;
5355

5456
public class DriverTests extends ESTestCase {
5557
/**
@@ -252,6 +254,58 @@ public void testProfileAndStatusInterval() {
252254
assertThat(driver.profile().cpuNanos(), equalTo(tickTime * inPages.size()));
253255
assertThat(driver.profile().iterations(), equalTo((long) inPages.size()));
254256
}
257+
public void testUnchangedStatus() {
258+
DriverContext driverContext = driverContext();
259+
List<Page> inPages = randomList(2, 100, DriverTests::randomPage);
260+
List<Page> outPages = new ArrayList<>();
261+
262+
long startEpoch = randomNonNegativeLong();
263+
long startNanos = randomLong();
264+
long waitTime = randomLongBetween(10000, 100000);
265+
long tickTime = randomLongBetween(10000, 100000);
266+
long statusInterval = randomLongBetween(1, 10);
267+
268+
Driver driver = new Driver(
269+
"unset",
270+
"test",
271+
startEpoch,
272+
startNanos,
273+
driverContext,
274+
() -> "unset",
275+
new CannedSourceOperator(inPages.iterator()),
276+
List.of(),
277+
new TestResultPageSinkOperator(outPages::add),
278+
TimeValue.timeValueNanos(statusInterval),
279+
() -> {}
280+
);
281+
282+
NowSupplier nowSupplier = new NowSupplier(startNanos, waitTime, tickTime);
283+
284+
int iterationsPerTick = randomIntBetween(1, 10);
285+
286+
for (int i = 0; i < inPages.size(); i += iterationsPerTick) {
287+
DriverStatus initialStatus = driver.status();
288+
long completedOperatorsHash = initialStatus.completedOperators().hashCode();
289+
long activeOperatorsHash = initialStatus.activeOperators().hashCode();
290+
long sleepsHash = initialStatus.sleeps().hashCode();
291+
292+
driver.run(TimeValue.timeValueDays(10), iterationsPerTick, nowSupplier);
293+
294+
DriverStatus newStatus = driver.status();
295+
assertThat(newStatus, not(sameInstance(initialStatus)));
296+
assertThat(
297+
newStatus.completedOperators() != initialStatus.completedOperators()
298+
|| newStatus.completedOperators().hashCode() == completedOperatorsHash,
299+
equalTo(true)
300+
);
301+
assertThat(
302+
newStatus.activeOperators() != initialStatus.activeOperators()
303+
|| newStatus.activeOperators().hashCode() == activeOperatorsHash,
304+
equalTo(true)
305+
);
306+
assertThat(newStatus.sleeps() != initialStatus.sleeps() || newStatus.sleeps().hashCode() == sleepsHash, equalTo(true));
307+
}
308+
}
255309

256310
class NowSupplier implements LongSupplier {
257311
private final long startNanos;

0 commit comments

Comments
 (0)