Cached statements hold per-run state, so two runs of the same SQL at once overwrite each other #2596

A cached compiled statement holds the state of one run, so two runs of the same SQL text at the same time overwrite each other. #1953 reports the Promise.all case and PR #2465 addresses that case; the cause underneath is wider. This issue is the whole picture, for whoever picks it up. It is planned for Phase 2 of the v5 roadmap, after the ES6 conversion, and is a prerequisite of the tagged template (#1723).

The cause in plain words

alasql compiles a query once per SQL text and keeps the compiled object in the cache. The object holds the compiled functions, which never change. But it is also used as a notepad while the query runs: the parameters of this call, the rows read so far, the groups being built, the callback to call at the end. There is one notepad per SQL text. When a second run of the same text starts before the first has finished, the second writes over the first one's notes.

In the code: Select.compile creates one query object and returns a function that closes over it. src/38query.js assigns about twenty fields on that object on every run (params, data, groups, xgroups, scope, cb, limit, offset, subqueryCache, …). Each source in query.sources gets its data written onto it too. cleanupCache exists to empty query.data afterwards.

Three reproductions on v4.20.0

All three use the same SQL text twice at once. Turning the cache off (alasql.options.cache = false) makes each one correct.

const alasql = require('alasql');
const j = JSON.stringify;

// 1. A user function that runs the same query again (walking a tree)
const tree = { id: 1, kids: [{ id: 2, kids: [{ id: 4, kids: [] }] }, { id: 3, kids: [] }] };
const q = 'SELECT id, kidsOf(kids) AS below FROM ? ORDER BY id';
alasql.fn.kidsOf = (kids) => (kids.length ? alasql(q, [kids]) : []);
console.log(j(alasql(q, [[tree]])));
// cache on:  []                       <- no error
// cache off: [{"id":1,"below":[{"id":2,"below":[{"id":4,"below":[]}]},{"id":3,"below":[]}]}]

// 2. The same with GROUP BY
const q2 = 'SELECT a, SUM(b) AS s, inner2(a) AS i FROM ? GROUP BY a';
alasql.fn.inner2 = (a) => (a < 2 ? alasql(q2, [[{ a: 9, b: 1 }, { a: 9, b: 2 }]]).length : 0);
alasql(q2, [[{ a: 1, b: 10 }, { a: 1, b: 5 }, { a: 2, b: 7 }]]);
// cache on:  throws "Cannot read properties of undefined (reading 'default')"
// cache off: [{"a":1,"s":15,"i":1},{"a":2,"s":7,"i":0}]

// 3. Two overlapping runs with an asynchronous source
alasql.from.SLOW = function (ms, opts, cb, idx, query) {
  setTimeout(() => cb && cb(opts.rows, idx, query), ms);
  return null;
};
const q3 = 'SELECT COLUMN x FROM SLOW(?, ?) ORDER BY x';
alasql.promise(q3, [30, { rows: [{ x: 1 }, { x: 2 }] }]).then((r) => console.log('first', j(r)));
alasql.promise(q3, [10, { rows: [{ x: 100 }, { x: 200 }, { x: 300 }] }]).then((r) => console.log('second', j(r)));
// cache on:  "second [100,200,300]"; the first promise never settles

Why it matters more in v5

The tagged template planned for v5 keeps one compiled statement per call site and makes the async form ordinary to use. Code like await Promise.all(ids.map((id) => alasql.async`… ${id} …`)) is exactly case 3. So the tag depends on this being fixed.

A proposal in three steps

1. Find every write. In a test build, freeze the compiled query object after compile (Object.freeze) and run the test suite. The source is in strict mode, so every write to shared state throws. The failures are the complete list of places to change.
2. Give each run its own object, without rewriting. At the start of the statement function, create const run = Object.create(query), and do the same for each source. Reads of compiled parts fall through to the shared object. Writes land on the run object, which is thrown away when the call ends. The hot loops stay as they are. cleanupCache and the query.query = query // TODO Remove to prevent memory leaks line are then no longer needed. Subqueries have their own query and need the same treatment.
3. Later, split properly into a plan (never changes) and an execution (the state of one run), once the source is in modules. Step 2 is correct without it.

Tests to add: the three cases above, the same with different parameters, and a check that a cached statement holds no reference to result data after it returns. Measure step 2 with test/performance/. The expected cost is one small allocation per source per run.

Related: #1953 (the Promise.all report), #2465 (fix for that case; its author should be credited), #1368 (the same report in 2021), #2587 (cache key collision), #1723 (the tag).