fix: stack overflow on large number of templates, #513

docs/themes/navy/layout/partial/all-contributors.swig

@@ -55,6 +55,7 @@
     <td align="center"><a href="https://github.com/ameyaapte1"><img src="https://avatars.githubusercontent.com/u/16054747?v=4?s=100" width="100px;" alt=""/></a></td>
     <td align="center"><a href="https://github.com/tbdrz"><img src="https://avatars.githubusercontent.com/u/50599116?v=4?s=100" width="100px;" alt=""/></a></td>
     <td align="center"><a href="http://santialbo.com"><img src="https://avatars.githubusercontent.com/u/1557563?v=4?s=100" width="100px;" alt=""/></a></td>
+    <td align="center"><a href="https://github.com/YahangWu"><img src="https://avatars.githubusercontent.com/u/12295975?v=4?s=100" width="100px;" alt=""/></a></td>
   </tr>
 </table>
 

src/cache/cache.ts

@@ -1,5 +1,9 @@
+import type { Template } from '../template/template'
+
 export interface Cache<T> {
   write (key: string, value: T): void | Promise<void>;
   read (key: string): T | undefined | Promise<T | undefined>;
   remove (key: string): void | Promise<void>;
 }
+
+export type LiquidCache = Cache<Template[] | Promise<Template[]>>

src/liquid-options.ts

@@ -1,12 +1,10 @@
 import { snakeCase, forOwn, isArray, isString, isFunction } from './util/underscore'
-import { Template } from './template/template'
-import { Cache } from './cache/cache'
+import { LiquidCache } from './cache/cache'
 import { LRU } from './cache/lru'
 import { FS } from './fs/fs'
 import * as fs from './fs/node'
 import { defaultOperators, Operators } from './render/operator'
 import { createTrie, Trie } from './util/operator-trie'
-import { Thenable } from './util/async'
 import * as builtinFilters from './builtin/filters'
 import { assert, FilterImplOptions } from './types'
 
@@ -32,7 +30,7 @@ export interface LiquidOptions {
   /** Add a extname (if filepath doesn't include one) before template file lookup. Eg: setting to `".html"` will allow including file by basename. Defaults to `""`. */
   extname?: string;
   /** Whether or not to cache resolved templates. Defaults to `false`. */
-  cache?: boolean | number | Cache<Thenable<Template[]>>;
+  cache?: boolean | number | LiquidCache;
   /** Use Javascript Truthiness. Defaults to `false`. */
   jsTruthy?: boolean;
   /** If set, treat the `filepath` parameter in `{%include filepath %}` and `{%layout filepath%}` as a variable, otherwise as a literal value. Defaults to `true`. */
@@ -104,7 +102,7 @@ interface NormalizedOptions extends LiquidOptions {
   root?: string[];
   partials?: string[];
   layouts?: string[];
-  cache?: Cache<Thenable<Template[]>>;
+  cache?: LiquidCache;
   outputEscape?: OutputEscape;
   operatorsTrie?: Trie;
 }
@@ -116,7 +114,7 @@ export interface NormalizedFullOptions extends NormalizedOptions {
   relativeReference: boolean;
   jekyllInclude: boolean;
   extname: string;
-  cache: undefined | Cache<Thenable<Template[]>>;
+  cache?: LiquidCache;
   jsTruthy: boolean;
   dynamicPartials: boolean;
   fs: FS;
@@ -180,7 +178,7 @@ export function normalize (options: LiquidOptions): NormalizedFullOptions {
     if (!options.hasOwnProperty('layouts')) options.layouts = options.root
   }
   if (options.hasOwnProperty('cache')) {
-    let cache: Cache<Thenable<Template[]>> | undefined
+    let cache: LiquidCache | undefined
     if (typeof options.cache === 'number') cache = options.cache > 0 ? new LRU(options.cache) : undefined
     else if (typeof options.cache === 'object') cache = options.cache
     else cache = options.cache ? new LRU(1024) : undefined

src/parser/parser.ts

@@ -8,17 +8,17 @@ import { Output } from '../template/output'
 import { HTML } from '../template/html'
 import { Template } from '../template/template'
 import { TopLevelToken } from '../tokens/toplevel-token'
-import { Cache } from '../cache/cache'
+import { LiquidCache } from '../cache/cache'
 import { Loader, LookupType } from '../fs/loader'
+import { toPromise } from '../util/async'
 import { FS } from '../fs/fs'
-import { toThenable, Thenable } from '../util/async'
 
 export default class Parser {
   public parseFile: (file: string, sync?: boolean, type?: LookupType, currentFile?: string) => Generator<unknown, Template[], Template[] | string>
 
   private liquid: Liquid
   private fs: FS
-  private cache: Cache<Thenable<Template[]>> | undefined
+  private cache?: LiquidCache
   private loader: Loader
 
   public constructor (liquid: Liquid) {
@@ -58,21 +58,18 @@ export default class Parser {
     return new ParseStream(tokens, (token, tokens) => this.parseToken(token, tokens))
   }
   private * _parseFileCached (file: string, sync?: boolean, type: LookupType = LookupType.Root, currentFile?: string): Generator<unknown, Template[], Template[]> {
-    const key = this.loader.shouldLoadRelative(file)
-      ? currentFile + ',' + file
-      : type + ':' + file
-    const tpls = yield this.cache!.read(key)
+    const cache = this.cache!
+    const key = this.loader.shouldLoadRelative(file) ? currentFile + ',' + file : type + ':' + file
+    const tpls = yield cache.read(key)
     if (tpls) return tpls
 
-    const task = toThenable(this._parseFile(file, sync, type, currentFile))
-    this.cache!.write(key, task)
-    try {
-      return yield task
-    } catch (e) {
-      // remove cached task if failed
-      this.cache!.remove(key)
-    }
-    return []
+    const task = this._parseFile(file, sync, type, currentFile)
+    // sync mode: exec the task and cache the result
+    // async mode: cache the task before exec
+    const taskOrTpl = sync ? yield task : toPromise(task)
+    cache.write(key, taskOrTpl as any)
+    // note: concurrent tasks will be reused, cache for failed task is removed until its end
+    try { return yield taskOrTpl } catch (err) { cache.remove(key); throw err }
   }
   private * _parseFile (file: string, sync?: boolean, type: LookupType = LookupType.Root, currentFile?: string): Generator<unknown, Template[], string> {
     const filepath = yield this.loader.lookup(file, type, sync, currentFile)

src/render/render.ts

@@ -4,13 +4,13 @@ import { Template } from '../template/template'
 import { Emitter } from '../emitters/emitter'
 import { SimpleEmitter } from '../emitters/simple-emitter'
 import { StreamedEmitter } from '../emitters/streamed-emitter'
-import { toThenable } from '../util/async'
+import { toPromise } from '../util/async'
 import { KeepingTypeEmitter } from '../emitters/keeping-type-emitter'
 
 export class Render {
   public renderTemplatesToNodeStream (templates: Template[], ctx: Context): NodeJS.ReadableStream {
     const emitter = new StreamedEmitter()
-    Promise.resolve().then(() => toThenable(this.renderTemplates(templates, ctx, emitter)))
+    Promise.resolve().then(() => toPromise(this.renderTemplates(templates, ctx, emitter)))
       .then(() => emitter.end(), err => emitter.error(err))
     return emitter.stream
   }

src/util/async.ts

@@ -7,25 +7,6 @@ export interface Thenable<T> {
   catch (reject: resolver): Thenable<T>;
 }
 
-function createResolvedThenable<T> (value: T): Thenable<T> {
-  const ret = {
-    then: (resolve: resolver) => resolve(value),
-    catch: () => ret
-  }
-  return ret
-}
-
-function createRejectedThenable<T> (err: Error): Thenable<T> {
-  const ret = {
-    then: (resolve: resolver, reject?: resolver) => {
-      if (reject) return reject(err)
-      return ret
-    },
-    catch: (reject: resolver) => reject(err)
-  }
-  return ret
-}
-
 function isThenable<T> (val: any): val is Thenable<T> {
   return val && isFunction(val.then)
 }
@@ -34,48 +15,49 @@ function isAsyncIterator (val: any): val is IterableIterator<any> {
   return val && isFunction(val.next) && isFunction(val.throw) && isFunction(val.return)
 }
 
-// convert an async iterator to a thenable (Promise compatible)
-export function toThenable<T> (val: IteratorResult<unknown, T> | Thenable<T> | any): Thenable<T> {
-  if (isThenable(val)) return val
-  if (isAsyncIterator(val)) return reduce()
-  return createResolvedThenable(val)
-
-  function reduce<T> (prev?: T): Thenable<T> {
-    let state
+// convert an async iterator to a Promise
+export async function toPromise<T> (val: Generator<unknown, T, unknown> | Thenable<T> | T): Promise<T> {
+  if (!isAsyncIterator(val)) return val
+  let value: unknown
+  let done = false
+  let next = 'next'
+  do {
+    const state = val[next](value)
+    done = state.done
+    value = state.value
+    next = 'next'
     try {
-      state = val.next(prev)
+      if (isAsyncIterator(value)) value = toPromise(value)
+      if (isThenable(value)) value = await value
     } catch (err) {
-      return createRejectedThenable(err as Error)
+      next = 'throw'
+      value = err
     }
-
-    if (state.done) return createResolvedThenable(state.value)
-    return toThenable(state.value!).then(reduce, err => {
-      let state
+  } while (!done)
+  return value as T
+}
+
+// convert an async iterator to a value in a synchronous maner
+export function toValue<T> (val: Generator<unknown, T, unknown> | T): T {
+  if (!isAsyncIterator(val)) return val
+  let value: any
+  let done = false
+  let next = 'next'
+  do {
+    const state = val[next](value)
+    done = state.done
+    value = state.value
+    next = 'next'
+    if (isAsyncIterator(value)) {
       try {
-        state = val.throw!(err)
-      } catch (e) {
-        return createRejectedThenable(e as Error)
+        value = toValue(value)
+      } catch (err) {
+        next = 'throw'
+        value = err
       }
-      if (state.done) return createResolvedThenable(state.value)
-      return reduce(state.value)
-    })
-  }
-}
-
-export function toPromise<T> (val: Generator<unknown, T, unknown> | Thenable<T> | T): Promise<T> {
-  return Promise.resolve(toThenable(val))
+    }
+  } while (!done)
+  return value
 }
 
-// get the value of async iterator in synchronous manner
-export function toValue<T> (val: Generator<unknown, T, unknown> | Thenable<T> | T): T {
-  let ret: T
-  toThenable(val)
-    .then((x: any) => {
-      ret = x
-      return createResolvedThenable(ret)
-    })
-    .catch((err: Error) => {
-      throw err
-    })
-  return ret!
-}
+export const toThenable = toPromise

test/e2e/issues.ts

@@ -246,4 +246,14 @@ describe('Issues', function () {
     const html = await engine.parseAndRender(`{% if template contains "product" %}contains{%endif%}`, ctx)
     expect(html).to.equal('contains')
   })
+  it('#513 should support large number of templates [async]', async () => {
+    const engine = new Liquid()
+    const html = await engine.parseAndRender(`{% for i in (1..10000) %}{{ i }}{% endfor %}`)
+    expect(html).to.have.lengthOf(38894)
+  })
+  it('#513 should support large number of templates [sync]', () => {
+    const engine = new Liquid()
+    const html = engine.parseAndRenderSync(`{% for i in (1..10000) %}{{ i }}{% endfor %}`)
+    expect(html).to.have.lengthOf(38894)
+  })
 })

test/integration/liquid/cache.ts

@@ -126,11 +126,13 @@ describe('LiquidOptions#cache', function () {
         extname: '.html',
         cache: true
       })
-      try { await engine.renderFile('foo') } catch (err) {}
+      try {
+        await engine.renderFile('foo')
+      } catch (err) {}
 
       mock({ '/root/foo.html': 'foo' })
-      const y = await engine.renderFile('foo')
-      expect(y).to.equal('foo')
+      const html = await engine.renderFile('foo')
+      expect(html).to.equal('foo')
     })
   })