From 8894cbfe6e0dbad4c07439adcefb6b3e2056be11 Mon Sep 17 00:00:00 2001 From: Harttle Date: Sat, 16 Oct 2021 20:35:20 +0800 Subject: [PATCH] fix: cache ongoing parseFile() calls, fixes #416 --- bin/perf-diff.sh | 6 +++--- package.json | 4 ++-- src/cache/cache.ts | 1 + src/liquid-options.ts | 11 ++++++----- src/parser/parser.ts | 20 +++++++++++++------- src/util/async.ts | 26 ++++++++++++++------------ test/e2e/issues.ts | 24 ++++++++++++++++++++++++ test/integration/liquid/cache.ts | 3 --- 8 files changed, 63 insertions(+), 32 deletions(-) diff --git a/bin/perf-diff.sh b/bin/perf-diff.sh index 6b33da8146..cdd2d78604 100755 --- a/bin/perf-diff.sh +++ b/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 diff --git a/package.json b/package.json index 80f69b8373..ef677d12a5 100644 --- a/package.json +++ b/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": { diff --git a/src/cache/cache.ts b/src/cache/cache.ts index 0ef661eff7..3ea13b89bb 100644 --- a/src/cache/cache.ts +++ b/src/cache/cache.ts @@ -1,4 +1,5 @@ export interface Cache { write (key: string, value: T): void | Promise; read (key: string): T | undefined | Promise; + remove (key: string): void | Promise; } diff --git a/src/liquid-options.ts b/src/liquid-options.ts index 2a7fcd4e94..2f0378e34a 100644 --- a/src/liquid-options.ts +++ b/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; + cache?: boolean | number | Cache>; /** 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; + cache?: Cache>; operatorsTrie?: Trie; } @@ -78,7 +79,7 @@ export interface NormalizedFullOptions extends NormalizedOptions { layouts: string[]; relativeReference: boolean; extname: string; - cache: undefined | Cache; + cache: undefined | Cache>; 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 | undefined + let cache: Cache> | 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 + else cache = options.cache ? new LRU(1024) : undefined options.cache = cache } if (options.hasOwnProperty('operators')) { diff --git a/src/parser/parser.ts b/src/parser/parser.ts index daa91b82f2..919b027836 100644 --- a/src/parser/parser.ts +++ b/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 private liquid: Liquid private fs: FS - private cache: Cache | undefined + private cache: Cache> | 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 { 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) } diff --git a/src/util/async.ts b/src/util/async.ts index 72c6e63c72..86afbf8780 100644 --- a/src/util/async.ts +++ b/src/util/async.ts @@ -2,12 +2,12 @@ import { isFunction } from './underscore' type resolver = (x?: any) => any -interface Thenable { - then (resolve: resolver, reject?: resolver): Thenable; - catch (reject: resolver): Thenable; +export interface Thenable { + then (resolve: resolver, reject?: resolver): Thenable; + catch (reject: resolver): Thenable; } -function createResolvedThenable (value: any): Thenable { +function createResolvedThenable (value: T): Thenable { const ret = { then: (resolve: resolver) => resolve(value), catch: () => ret @@ -15,7 +15,7 @@ function createResolvedThenable (value: any): Thenable { return ret } -function createRejectedThenable (err: Error): Thenable { +function createRejectedThenable (err: Error): Thenable { const ret = { then: (resolve: resolver, reject?: resolver) => { if (reject) return reject(err) @@ -26,7 +26,7 @@ function createRejectedThenable (err: Error): Thenable { return ret } -function isThenable (val: any): val is Thenable { +function isThenable (val: any): val is Thenable { return val && isFunction(val.then) } @@ -34,13 +34,15 @@ function isAsyncIterator (val: any): val is IterableIterator { return val && isFunction(val.next) && isFunction(val.throw) && isFunction(val.return) } +type Task = Thenable + // convert an async iterator to a thenable (Promise compatible) -export function toThenable (val: IterableIterator | Thenable | any): Thenable { +export function toThenable (val: IterableIterator | Thenable | any): Thenable { if (isThenable(val)) return val if (isAsyncIterator(val)) return reduce() return createResolvedThenable(val) - function reduce (prev?: any): Thenable { + function reduce (prev?: T): Thenable { let state try { state = (val as IterableIterator).next(prev) @@ -62,13 +64,13 @@ export function toThenable (val: IterableIterator | Thenable | any): Thenab } } -export function toPromise (val: IterableIterator | Thenable | any): Promise { +export function toPromise (val: IterableIterator | Thenable | T): Promise { return Promise.resolve(toThenable(val)) } // get the value of async iterator in synchronous manner -export function toValue (val: IterableIterator | Thenable | any) { - let ret: any +export function toValue (val: IterableIterator | Thenable | T): T { + let ret: T toThenable(val) .then((x: any) => { ret = x @@ -77,5 +79,5 @@ export function toValue (val: IterableIterator | Thenable | any) { .catch((err: Error) => { throw err }) - return ret + return ret! } diff --git a/test/e2e/issues.ts b/test/e2e/issues.ts index 31e947ffaf..199c120de3 100644 --- a/test/e2e/issues.ts +++ b/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 + }) }) diff --git a/test/integration/liquid/cache.ts b/test/integration/liquid/cache.ts index 0f6a3125e8..08a2f0e05c 100644 --- a/test/integration/liquid/cache.ts +++ b/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') })