fix: cache ongoing parseFile() calls, fixes #416

bin/perf-diff.sh

@@ -9,8 +9,8 @@ if [ ! -f $FILE_LATEST ]; then
     curl $URL_LATEST > $FILE_LATEST
 fi
 
-if [ ! -f $FILE_LOCAL ]; then
-    BUNDLES=cjs npm run build:dist
-fi
+# if [ ! -f $FILE_LOCAL ]; then
+BUNDLES=cjs npm run build:dist
+# fi
 
 exec node benchmark/diff.js $FILE_LOCAL $FILE_LATEST

package.json

@@ -12,14 +12,14 @@
   "types": "dist/liquid.d.ts",
   "scripts": {
     "lint": "eslint \"**/*.ts\" .",
-    "check": "npm test && npm run lint",
+    "check": "npm run build && npm test && npm run lint",
     "test": "nyc mocha \"test/**/*.ts\"",
     "test:e2e": "mocha \"test/e2e/**/*.ts\"",
     "perf": "cd benchmark && npm ci && npm start",
     "perf:diff": "bin/perf-diff.sh",
     "perf:engines": "cd benchmark && npm run engines",
     "build": "npm run build:dist && npm run build:docs",
-    "build:dist": "rollup -c rollup.config.ts && ls -lh dist",
+    "build:dist": "rollup -c rollup.config.ts",
     "build:docs": "bin/build-docs.sh"
   },
   "bin": {

src/cache/cache.ts

@@ -1,4 +1,5 @@
 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>;
 }

src/liquid-options.ts

@@ -6,6 +6,7 @@ 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'
 
 export interface LiquidOptions {
   /** A directory or an array of directories from where to resolve layout and include templates, and the filename passed to `.renderFile()`. If it's an array, the files are looked up in the order they occur in the array. Defaults to `["."]` */
@@ -19,7 +20,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<Template[]>;
+  cache?: boolean | number | Cache<Thenable<Template[]>>;
   /** 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`. */
@@ -68,7 +69,7 @@ interface NormalizedOptions extends LiquidOptions {
   root?: string[];
   partials?: string[];
   layouts?: string[];
-  cache?: Cache<Template[]>;
+  cache?: Cache<Thenable<Template[]>>;
   operatorsTrie?: Trie;
 }
 
@@ -78,7 +79,7 @@ export interface NormalizedFullOptions extends NormalizedOptions {
   layouts: string[];
   relativeReference: boolean;
   extname: string;
-  cache: undefined | Cache<Template[]>;
+  cache: undefined | Cache<Thenable<Template[]>>;
   jsTruthy: boolean;
   dynamicPartials: boolean;
   fs: FS;
@@ -142,10 +143,10 @@ export function normalize (options?: LiquidOptions): NormalizedOptions {
     options.layouts = normalizeDirectoryList(options.layouts)
   }
   if (options.hasOwnProperty('cache')) {
-    let cache: Cache<Template[]> | undefined
+    let cache: Cache<Thenable<Template[]>> | 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<Template[]>(1024) : undefined
+    else cache = options.cache ? new LRU(1024) : undefined
     options.cache = cache
   }
   if (options.hasOwnProperty('operators')) {

src/parser/parser.ts

@@ -11,13 +11,14 @@ import { TopLevelToken } from '../tokens/toplevel-token'
 import { Cache } from '../cache/cache'
 import { Loader, LookupType } from '../fs/loader'
 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) => Iterator<Template[]>
 
   private liquid: Liquid
   private fs: FS
-  private cache: Cache<Template[]> | undefined
+  private cache: Cache<Thenable<Template[]>> | undefined
   private loader: Loader
 
   public constructor (liquid: Liquid) {
@@ -60,14 +61,19 @@ export default class Parser {
     const key = this.loader.shouldLoadRelative(file)
       ? currentFile + ',' + file
       : type + ':' + file
-    let templates = yield this.cache!.read(key)
-    if (templates) return templates
+    const tpls = yield this.cache!.read(key)
+    if (tpls) return tpls
 
-    templates = yield this._parseFile(file, sync, type, currentFile)
-    this.cache!.write(key, templates)
-    return templates
+    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)
+    }
   }
-  private * _parseFile (file: string, sync?: boolean, type: LookupType = LookupType.Root, currentFile?: string) {
+  private * _parseFile (file: string, sync?: boolean, type: LookupType = LookupType.Root, currentFile?: string): IterableIterator<any> {
     const filepath = yield this.loader.lookup(file, type, sync, currentFile)
     return this.liquid.parse(sync ? this.fs.readFileSync(filepath) : yield this.fs.readFile(filepath), filepath)
   }

src/util/async.ts

@@ -2,20 +2,20 @@ import { isFunction } from './underscore'
 
 type resolver = (x?: any) => any
 
-interface Thenable {
-  then (resolve: resolver, reject?: resolver): Thenable;
-  catch (reject: resolver): Thenable;
+export interface Thenable<T> {
+  then (resolve: resolver, reject?: resolver): Thenable<T>;
+  catch (reject: resolver): Thenable<T>;
 }
 
-function createResolvedThenable (value: any): Thenable {
+function createResolvedThenable<T> (value: T): Thenable<T> {
   const ret = {
     then: (resolve: resolver) => resolve(value),
     catch: () => ret
   }
   return ret
 }
 
-function createRejectedThenable (err: Error): Thenable {
+function createRejectedThenable<T> (err: Error): Thenable<T> {
   const ret = {
     then: (resolve: resolver, reject?: resolver) => {
       if (reject) return reject(err)
@@ -26,21 +26,23 @@ function createRejectedThenable (err: Error): Thenable {
   return ret
 }
 
-function isThenable (val: any): val is Thenable {
+function isThenable<T> (val: any): val is Thenable<T> {
   return val && isFunction(val.then)
 }
 
 function isAsyncIterator (val: any): val is IterableIterator<any> {
   return val && isFunction(val.next) && isFunction(val.throw) && isFunction(val.return)
 }
 
+type Task<T> = Thenable<T>
+
 // convert an async iterator to a thenable (Promise compatible)
-export function toThenable (val: IterableIterator<any> | Thenable | any): Thenable {
+export function toThenable<T> (val: IterableIterator<T> | Thenable<T> | any): Thenable<T> {
   if (isThenable(val)) return val
   if (isAsyncIterator(val)) return reduce()
   return createResolvedThenable(val)
 
-  function reduce (prev?: any): Thenable {
+  function reduce<T> (prev?: T): Thenable<T> {
     let state
     try {
       state = (val as IterableIterator<any>).next(prev)
@@ -62,13 +64,13 @@ export function toThenable (val: IterableIterator<any> | Thenable | any): Thenab
   }
 }
 
-export function toPromise (val: IterableIterator<any> | Thenable | any): Promise<any> {
+export function toPromise<T> (val: IterableIterator<T> | Thenable<T> | T): Promise<T> {
   return Promise.resolve(toThenable(val))
 }
 
 // get the value of async iterator in synchronous manner
-export function toValue (val: IterableIterator<any> | Thenable | any) {
-  let ret: any
+export function toValue<T> (val: IterableIterator<T> | Thenable<T> | T): T {
+  let ret: T
   toThenable(val)
     .then((x: any) => {
       ret = x
@@ -77,5 +79,5 @@ export function toValue (val: IterableIterator<any> | Thenable | any) {
     .catch((err: Error) => {
       throw err
     })
-  return ret
+  return ret!
 }

test/e2e/issues.ts

@@ -1,9 +1,12 @@
 import { Liquid } from '../../src/liquid'
 import { expect, use } from 'chai'
 import * as chaiAsPromised from 'chai-as-promised'
+import * as sinon from 'sinon'
+import * as sinonChai from 'sinon-chai'
 const LiquidUMD = require('../../dist/liquid.browser.umd.js').Liquid
 
 use(chaiAsPromised)
+use(sinonChai)
 
 describe('Issues', function () {
   it('#221 unicode blanks are not properly treated', async () => {
@@ -129,4 +132,25 @@ describe('Issues', function () {
     const html = await engine.renderSync(tpl)
     expect(html).to.equal('/tmp/foo.liquid')
   })
+  it('#416 Templates imported by {% render %} not cached for concurrent async render', async () => {
+    const readFile = sinon.spy(() => Promise.resolve('HELLO'))
+    const exists = sinon.spy(() => 'HELLO')
+    const engine = new Liquid({
+      cache: true,
+      extname: '.liquid',
+      root: '~',
+      fs: {
+        exists,
+        resolve: (root: string, file: string, ext: string) => root + '#' + file + ext,
+        sep: '#',
+        readFile
+      } as any
+    })
+
+    await Promise.all(Array(5).fill(0).map(
+      x => engine.parseAndRender("{% render 'template' %}")
+    ))
+    expect(exists).to.be.calledOnce
+    expect(readFile).to.be.calledOnce
+  })
 })

test/integration/liquid/cache.ts

@@ -155,10 +155,7 @@ describe('LiquidOptions#cache', function () {
         cache: true
       })
       mock({ '/root/foo.html': 'foo' })
-      mock({ '/root/bar.html': 'bar' })
-
       expect(engine.renderFileSync('foo')).to.equal('foo')
-      expect(engine.renderFileSync('bar')).to.equal('bar')
       mock({ '/root/foo.html': 'bar' })
       expect(engine.renderFileSync('foo')).to.equal('foo')
     })