Repository: calcite Updated Branches: refs/heads/master de3880298 -> a850c41e2
[CALCITE-1009] SelfPopulatingList is not thread-safe Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/a850c41e Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/a850c41e Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/a850c41e Branch: refs/heads/master Commit: a850c41e2da262234475ec56383457475e9fabee Parents: de38802 Author: Julian Hyde <[email protected]> Authored: Tue Dec 8 21:40:01 2015 -0800 Committer: Julian Hyde <[email protected]> Committed: Tue Dec 8 23:02:10 2015 -0800 ---------------------------------------------------------------------- .../java/org/apache/calcite/rex/RexSlot.java | 11 +++-- .../org/apache/calcite/rex/RexExecutorTest.java | 46 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/a850c41e/core/src/main/java/org/apache/calcite/rex/RexSlot.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rex/RexSlot.java b/core/src/main/java/org/apache/calcite/rex/RexSlot.java index 5421f12..d56db42 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSlot.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSlot.java @@ -89,9 +89,14 @@ public abstract class RexSlot extends RexVariable { if (index < 0) { throw new IllegalArgumentException(); } - addAll( - fromTo( - prefix, size(), Math.max(index + 1, size() * 2))); + // Double-checked locking, but safe because CopyOnWriteArrayList.array + // is marked volatile, and size() uses array.length. + synchronized (this) { + final int size = size(); + if (index >= size) { + addAll(fromTo(prefix, size, Math.max(index + 1, size * 2))); + } + } } } } http://git-wip-us.apache.org/repos/asf/calcite/blob/a850c41e/core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java b/core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java index d4780a6..72a36e2 100644 --- a/core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java +++ b/core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java @@ -41,9 +41,11 @@ import java.math.BigDecimal; import java.util.ArrayList; import java.util.Calendar; import java.util.List; +import java.util.Random; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -211,6 +213,50 @@ public class RexExecutorTest { }); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-1009";>[CALCITE-1009] + * SelfPopulatingList is not thread-safe</a>. */ + @Test public void testSelfPopulatingList() { + final List<Thread> threads = new ArrayList<>(); + //noinspection MismatchedQueryAndUpdateOfCollection + final List<String> list = new RexSlot.SelfPopulatingList("$", 1); + final Random random = new Random(); + for (int i = 0; i < 10; i++) { + threads.add( + new Thread() { + public void run() { + for (int j = 0; j < 1000; j++) { + // Random numbers between 0 and ~1m, smaller values more common + final int index = random.nextInt(1234567) + >> random.nextInt(16) >> random.nextInt(16); + list.get(index); + } + } + }); + } + for (Thread runnable : threads) { + runnable.start(); + } + for (Thread runnable : threads) { + try { + runnable.join(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + } + final int size = list.size(); + for (int i = 0; i < size; i++) { + assertThat(list.get(i), is("$" + i)); + } + } + + @Test public void testSelfPopulatingList30() { + //noinspection MismatchedQueryAndUpdateOfCollection + final List<String> list = new RexSlot.SelfPopulatingList("$", 30); + final String s = list.get(30); + assertThat(s, is("$30")); + } + /** Callback for {@link #check}. Test code will typically use {@code builder} * to create some expressions, call * {@link org.apache.calcite.rex.RexExecutorImpl#reduce} to evaluate them into
