From db95e7bc7cdbb6929d4be6dc605ccf3a3cb7a17e Mon Sep 17 00:00:00 2001 From: Aria Stewart Date: Mon, 18 May 2015 13:49:32 -0400 Subject: [PATCH 01/19] Resolve paths for views asynchronously --- lib/application.js | 29 ++++++----- lib/view.js | 118 ++++++++++++++++++++++++++++----------------- 2 files changed, 92 insertions(+), 55 deletions(-) diff --git a/lib/application.js b/lib/application.js index 310e6dfef21..d298c17aa53 100644 --- a/lib/application.js +++ b/lib/application.js @@ -554,25 +554,30 @@ app.render = function render(name, options, callback) { root: this.get('views'), engines: engines }); + } - if (!view.path) { - var dirs = Array.isArray(view.root) && view.root.length > 1 - ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' - : 'directory "' + view.root + '"' - var err = new Error('Failed to lookup view "' + name + '" in views ' + dirs); - err.view = view; - return done(err); - } + if (!view.path) { + view.lookupMain(function (err) { + if (err) return done(err); + // prime the cache + if (renderOptions.cache) { + cache[name] = view; + } + + // render + tryRender(view, renderOptions, done); + }); + } else { // prime the cache if (renderOptions.cache) { cache[name] = view; } - } - // render - tryRender(view, renderOptions, done); -}; + // render + tryRender(view, renderOptions, done); + } +} /** * Listen for connections. diff --git a/lib/view.js b/lib/view.js index d66b4a2d89c..234558e9375 100644 --- a/lib/view.js +++ b/lib/view.js @@ -89,26 +89,30 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; - - // lookup path - this.path = this.lookup(fileName); } /** - * Lookup view by the given `name` + * Lookup view by the given `name` and `ext` * * @param {string} name + * @param {String} ext + * @param {Function} cb * @private */ -View.prototype.lookup = function lookup(name) { - var path; +View.prototype.lookup = function lookup(name, cb) { var roots = [].concat(this.root); debug('lookup "%s"', name); - for (var i = 0; i < roots.length && !path; i++) { - var root = roots[i]; + var ext = this.ext; + + function lookup(roots, callback) { + var root = roots.shift(); + if (!root) { + return callback(null, null); + } + debug("looking up '%s' in '%s'", name, root); // resolve the path var loc = resolve(root, name); @@ -116,10 +120,19 @@ View.prototype.lookup = function lookup(name) { var file = basename(loc); // resolve the file - path = this.resolve(dir, file); + resolveView(dir, file, ext, function (err, resolved) { + if (err) { + return callback(err); + } else if (resolved) { + return callback(null, resolved); + } else { + return lookup(roots, callback); + } + }); + } - return path; + return lookup(roots, cb); }; /** @@ -135,6 +148,8 @@ View.prototype.render = function render(options, callback) { debug('render "%s"', this.path); + if (!this.path) return fn(new Error("View has not been fully initialized yet")); + // render, normalizing sync callbacks this.engine(this.path, options, function onRender() { if (!sync) { @@ -158,48 +173,65 @@ View.prototype.render = function render(options, callback) { sync = false; }; -/** - * Resolve the file within the given directory. +/** Resolve the main template for this view * - * @param {string} dir - * @param {string} file + * @param {function} cb * @private */ - -View.prototype.resolve = function resolve(dir, file) { - var ext = this.ext; - - // . - var path = join(dir, file); - var stat = tryStat(path); - - if (stat && stat.isFile()) { - return path; - } - - // /index. - path = join(dir, basename(file, ext), 'index' + ext); - stat = tryStat(path); - - if (stat && stat.isFile()) { - return path; - } +View.prototype.lookupMain = function lookupMain(cb) { + if (this.path) return cb(); + var view = this; + var name = path.extname(this.name) === this.ext + ? this.name + : this.name + this.ext; + this.lookup(name, function (err, path) { + if (err) { + return cb(err); + } else if (!path) { + var dirs = Array.isArray(view.root) && view.root.length > 1 + ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' + : 'directory "' + view.root + '"' + var viewError = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); + viewError.view = view; + return cb(viewError); + } else { + view.path = path; + cb(); + } + }); }; /** - * Return a stat, maybe. + * Resolve the file within the given directory. * - * @param {string} path - * @return {fs.Stats} + * @param {string} dir + * @param {string} file + * @param {string} ext + * @param {function} cb * @private */ -function tryStat(path) { - debug('stat "%s"', path); +function resolveView(dir, file, ext, cb) { + var path = join(dir, file); - try { - return fs.statSync(path); - } catch (e) { - return undefined; - } + // . + fs.stat(path, function (err, stat) { + if (err && err.code !== 'ENOENT') { + return cb(err); + } else if (!err && stat && stat.isFile()) { + return cb(null, path); + } + + // /index. + path = join(dir, basename(file, ext), 'index' + ext); + fs.stat(path, function (err, stat) { + if (err && err.code === 'ENOENT') { + return cb(null, null); + } else if (!err && stat && stat.isFile()) { + return cb(null, path); + } else { + return cb(err || new Error("error looking up '" + path + "'")); + } + }); + }); } From 3dd0d303852a7a17fa1be39f2355cf48e573241b Mon Sep 17 00:00:00 2001 From: Aria Stewart Date: Mon, 18 May 2015 15:05:39 -0400 Subject: [PATCH 02/19] Add limitStat to keep concurrent requests under control --- lib/view.js | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/lib/view.js b/lib/view.js index 234558e9375..ef48c6a6c5d 100644 --- a/lib/view.js +++ b/lib/view.js @@ -61,15 +61,11 @@ function View(name, options) { throw new Error('No default engine was specified and no extension was provided.'); } - var fileName = name; - if (!this.ext) { // get extension from default engine name this.ext = this.defaultEngine[0] !== '.' ? '.' + this.defaultEngine : this.defaultEngine; - - fileName += this.ext; } if (!opts.engines[this.ext]) { @@ -215,7 +211,7 @@ function resolveView(dir, file, ext, cb) { var path = join(dir, file); // . - fs.stat(path, function (err, stat) { + limitStat(path, function (err, stat) { if (err && err.code !== 'ENOENT') { return cb(err); } else if (!err && stat && stat.isFile()) { @@ -224,7 +220,7 @@ function resolveView(dir, file, ext, cb) { // /index. path = join(dir, basename(file, ext), 'index' + ext); - fs.stat(path, function (err, stat) { + limitStat(path, function (err, stat) { if (err && err.code === 'ENOENT') { return cb(null, null); } else if (!err && stat && stat.isFile()) { @@ -235,3 +231,31 @@ function resolveView(dir, file, ext, cb) { }); }); } + +var pendingStats = []; +var numPendingStats = 0; +/** + * an fs.stat call that limits the number of outstanding requests to 10. + * + * @param {String} path + * @param {Function} cb + */ +function limitStat(path, cb) { + if (++numPendingStats > 10) { + pendingStats.push([path, cb]); + } else { + fs.stat(path, cbAndDequeue(cb)); + } + + function cbAndDequeue(cb) { + return function (err, stat) { + cb(err, stat); + var next = pendingStats.shift(); + if (next) { + fs.stat(next[0], cbAndDequeue(next[1])); + } else { + numPendingStats--; + } + } + } +} From 0594ee6414cfe20c7a15296785e59e8b212c1301 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 19:46:30 -0500 Subject: [PATCH 03/19] fix(view): prevent limitStat queue stall when a callback throws --- lib/view.js | 53 ++++++++++++++++++++++++------------- test/app.render.js | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/lib/view.js b/lib/view.js index ef48c6a6c5d..98cbd72c6b6 100644 --- a/lib/view.js +++ b/lib/view.js @@ -232,30 +232,47 @@ function resolveView(dir, file, ext, cb) { }); } +/** + * Module variables for stat concurrency limiting. + * @private + */ + +var MAX_PENDING_STATS = 10; var pendingStats = []; var numPendingStats = 0; + /** - * an fs.stat call that limits the number of outstanding requests to 10. + * An fs.stat call that limits the number of outstanding requests. * - * @param {String} path - * @param {Function} cb + * @param {string} path + * @param {function} cb + * @private */ + function limitStat(path, cb) { - if (++numPendingStats > 10) { - pendingStats.push([path, cb]); - } else { - fs.stat(path, cbAndDequeue(cb)); - } + pendingStats.push([path, cb]); + statNext(); +} - function cbAndDequeue(cb) { - return function (err, stat) { - cb(err, stat); - var next = pendingStats.shift(); - if (next) { - fs.stat(next[0], cbAndDequeue(next[1])); - } else { - numPendingStats--; - } - } +function statNext() { + if (numPendingStats >= MAX_PENDING_STATS || pendingStats.length === 0) { + return; } + + var next = pendingStats.shift(); + var path = next[0]; + var cb = next[1]; + + numPendingStats++; + debug('stat "%s"', path); + + fs.stat(path, function onStat(err, stat) { + numPendingStats--; + + // dispatch the next queued stat before invoking the callback so a + // throwing callback cannot stall the queue + statNext(); + + cb(err, stat); + }); } diff --git a/test/app.render.js b/test/app.render.js index bd65ce1035b..57909a55e61 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -2,6 +2,7 @@ var assert = require('node:assert') var express = require('..'); +var fs = require('node:fs') var path = require('node:path') var tmpl = require('./support/tmpl'); @@ -381,6 +382,71 @@ describe('app', function(){ }) }) }) + + describe('view lookup concurrency', function () { + it('should complete when more renders are in flight than the stat limit', function (done) { + var app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.locals.user = { name: 'tobi' }; + + var remaining = 30; + + for (var i = 0; i < 30; i++) { + app.render('user.tmpl', function (err, str) { + if (err) return done(err); + assert.strictEqual(str, '

tobi

') + if (--remaining === 0) done(); + }) + } + }) + + describe('when a render callback throws', function () { + it('should not stall subsequent lookups', function (done) { + var app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.locals.user = { name: 'tobi' }; + + // stub fs.stat to call back synchronously so the throw below + // propagates to this stack frame instead of crashing the process + var realStat = fs.stat; + fs.stat = function stubStat(path, cb) { + var err = new Error('stubbed ENOENT'); + err.code = 'ENOENT'; + cb(err); + }; + + var thrown = 0; + + try { + // exhaust the stat limit with callbacks that throw + for (var i = 0; i < 12; i++) { + try { + app.render('does-not-exist.tmpl', function () { + throw new Error('boom'); + }) + } catch (err) { + assert.strictEqual(err.message, 'boom') + thrown++; + } + } + } finally { + fs.stat = realStat; + } + + // every callback must have run despite the throws + assert.strictEqual(thrown, 12) + + // a later render must still complete + app.render('user.tmpl', function (err, str) { + if (err) return done(err); + assert.strictEqual(str, '

tobi

') + done(); + }) + }) + }) + }) }) function createApp() { From f5ad5a3923ed206d0a5bbc413ddadd223d6b5f22 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 19:51:40 -0500 Subject: [PATCH 04/19] fix(view): treat stat errors as lookup misses like the sync implementation --- lib/view.js | 35 ++++++++++++++++++----------------- test/app.render.js | 17 +++++++++++++++++ 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/lib/view.js b/lib/view.js index 98cbd72c6b6..a6e29fdede0 100644 --- a/lib/view.js +++ b/lib/view.js @@ -208,28 +208,29 @@ View.prototype.lookupMain = function lookupMain(cb) { */ function resolveView(dir, file, ext, cb) { - var path = join(dir, file); + var filePath = join(dir, file); + var indexPath = join(dir, basename(file, ext), 'index' + ext); // . - limitStat(path, function (err, stat) { - if (err && err.code !== 'ENOENT') { - return cb(err); - } else if (!err && stat && stat.isFile()) { - return cb(null, path); + limitStat(filePath, onFileStat); + + function onFileStat(err, stat) { + if (!err && stat.isFile()) { + return cb(null, filePath); } // /index. - path = join(dir, basename(file, ext), 'index' + ext); - limitStat(path, function (err, stat) { - if (err && err.code === 'ENOENT') { - return cb(null, null); - } else if (!err && stat && stat.isFile()) { - return cb(null, path); - } else { - return cb(err || new Error("error looking up '" + path + "'")); - } - }); - }); + limitStat(indexPath, onIndexStat); + } + + function onIndexStat(err, stat) { + if (!err && stat.isFile()) { + return cb(null, indexPath); + } + + // treat any stat error as a miss, like the sync implementation did + cb(null, null); + } } /** diff --git a/test/app.render.js b/test/app.render.js index 57909a55e61..3689fca9c12 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -185,6 +185,23 @@ describe('app', function(){ }) }) + it('should ignore stat errors and keep looking up', function(done){ + var app = createApp(); + var views = [ + path.join(__dirname, 'fixtures', 'user.tmpl'), // a file: lookups inside it fail with ENOTDIR + path.join(__dirname, 'fixtures') + ] + + app.set('views', views); + app.locals.user = { name: 'tobi' }; + + app.render('user.tmpl', function (err, str) { + if (err) return done(err); + assert.strictEqual(str, '

tobi

') + done(); + }) + }) + it('should error if file does not exist', function(done){ var app = createApp(); var views = [ From 48b658848c99ecfbe1fd7a8a9c9203837bc68429 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 19:54:58 -0500 Subject: [PATCH 05/19] refactor(view): resolve the view path lazily in View#render --- lib/application.js | 22 ++------ lib/view.js | 124 +++++++++++++++++++++++++++------------------ 2 files changed, 79 insertions(+), 67 deletions(-) diff --git a/lib/application.js b/lib/application.js index d298c17aa53..cd46ad944d5 100644 --- a/lib/application.js +++ b/lib/application.js @@ -554,30 +554,16 @@ app.render = function render(name, options, callback) { root: this.get('views'), engines: engines }); - } - - if (!view.path) { - view.lookupMain(function (err) { - if (err) return done(err); - // prime the cache - if (renderOptions.cache) { - cache[name] = view; - } - - // render - tryRender(view, renderOptions, done); - }); - } else { // prime the cache if (renderOptions.cache) { cache[name] = view; } - - // render - tryRender(view, renderOptions, done); } -} + + // render + tryRender(view, renderOptions, done); +}; /** * Listen for connections. diff --git a/lib/view.js b/lib/view.js index a6e29fdede0..26b3bfb7fd9 100644 --- a/lib/view.js +++ b/lib/view.js @@ -61,11 +61,15 @@ function View(name, options) { throw new Error('No default engine was specified and no extension was provided.'); } + var fileName = name; + if (!this.ext) { // get extension from default engine name this.ext = this.defaultEngine[0] !== '.' ? '.' + this.defaultEngine : this.defaultEngine; + + fileName += this.ext; } if (!opts.engines[this.ext]) { @@ -85,69 +89,102 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; + + // file name to lookup on first render + this.fileName = fileName; } /** - * Lookup view by the given `name` and `ext` + * Lookup view by the given `name` * * @param {string} name - * @param {String} ext - * @param {Function} cb + * @param {function} cb * @private */ View.prototype.lookup = function lookup(name, cb) { var roots = [].concat(this.root); + var ext = this.ext; debug('lookup "%s"', name); - var ext = this.ext; + tryNextRoot(); - function lookup(roots, callback) { + function tryNextRoot() { var root = roots.shift(); + if (!root) { - return callback(null, null); + return cb(null, null); } - debug("looking up '%s' in '%s'", name, root); // resolve the path var loc = resolve(root, name); - var dir = dirname(loc); - var file = basename(loc); // resolve the file - resolveView(dir, file, ext, function (err, resolved) { - if (err) { - return callback(err); - } else if (resolved) { - return callback(null, resolved); - } else { - return lookup(roots, callback); - } - }); - + resolveView(dirname(loc), basename(loc), ext, onResolved); } - return lookup(roots, cb); + function onResolved(err, filePath) { + if (err || filePath) { + return cb(err, filePath); + } + + // not found; try the next root + tryNextRoot(); + } }; /** * Render with the given options. * + * Resolves the view path on first render and memoizes it + * on the instance for subsequent renders. + * * @param {object} options * @param {function} callback * @private */ View.prototype.render = function render(options, callback) { - var sync = true; + var view = this; - debug('render "%s"', this.path); + if (this.path) { + return renderFile(this, options, callback); + } - if (!this.path) return fn(new Error("View has not been fully initialized yet")); + // lookup path on first render + this.lookup(this.fileName, onLookup); + + function onLookup(err, filePath) { + if (err) { + return callback(err); + } + + if (!filePath) { + return callback(lookupFailedError(view)); + } + + view.path = filePath; + renderFile(view, options, callback); + } +}; + +/** + * Render the resolved view path with the given options. + * + * @param {View} view + * @param {object} options + * @param {function} callback + * @private + */ + +function renderFile(view, options, callback) { + var sync = true; + + debug('render "%s"', view.path); // render, normalizing sync callbacks - this.engine(this.path, options, function onRender() { + view.engine(view.path, options, function onRender() { if (!sync) { return callback.apply(this, arguments); } @@ -167,35 +204,24 @@ View.prototype.render = function render(options, callback) { }); sync = false; -}; +} -/** Resolve the main template for this view +/** + * Build the error for a failed view lookup. * - * @param {function} cb + * @param {View} view + * @return {Error} * @private */ -View.prototype.lookupMain = function lookupMain(cb) { - if (this.path) return cb(); - var view = this; - var name = path.extname(this.name) === this.ext - ? this.name - : this.name + this.ext; - this.lookup(name, function (err, path) { - if (err) { - return cb(err); - } else if (!path) { - var dirs = Array.isArray(view.root) && view.root.length > 1 - ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' - : 'directory "' + view.root + '"' - var viewError = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); - viewError.view = view; - return cb(viewError); - } else { - view.path = path; - cb(); - } - }); -}; + +function lookupFailedError(view) { + var dirs = Array.isArray(view.root) && view.root.length > 1 + ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' + : 'directory "' + view.root + '"' + var err = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); + err.view = view; + return err; +} /** * Resolve the file within the given directory. From 4786b4e6dfa24eba0e96a321df283b499451bee1 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 20:08:57 -0500 Subject: [PATCH 06/19] fix(view): deliver sync engine throws via callback on first render --- lib/view.js | 17 ++++++++++++----- test/app.render.js | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/lib/view.js b/lib/view.js index 26b3bfb7fd9..1457e51b6f2 100644 --- a/lib/view.js +++ b/lib/view.js @@ -183,8 +183,17 @@ function renderFile(view, options, callback) { debug('render "%s"', view.path); - // render, normalizing sync callbacks - view.engine(view.path, options, function onRender() { + try { + // render, normalizing sync callbacks + view.engine(view.path, options, onRender); + } catch (err) { + // deliver sync engine throws through the callback + return onRender(err); + } + + sync = false; + + function onRender() { if (!sync) { return callback.apply(this, arguments); } @@ -201,9 +210,7 @@ function renderFile(view, options, callback) { return process.nextTick(function renderTick() { return callback.apply(cntx, args); }); - }); - - sync = false; + } } /** diff --git a/test/app.render.js b/test/app.render.js index 3689fca9c12..db9c761f04c 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -105,6 +105,21 @@ describe('app', function(){ done() }) }) + + it('should pass a sync engine throw to the callback on first render', function(done){ + var app = express(); + + app.engine('tmpl', function () { + throw new Error('oops') + }) + app.set('views', path.join(__dirname, 'fixtures')) + + app.render('user.tmpl', function (err) { + assert.ok(err) + assert.strictEqual(err.message, 'oops') + done() + }) + }) }) describe('when an extension is given', function(){ From d08d353429aa7be0abfbd1c898eaf69a6bd42436 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 20:19:04 -0500 Subject: [PATCH 07/19] docs! --- History.md | 2 ++ test/app.render.js | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/History.md b/History.md index 8a4293ab547..e37c98fc24d 100644 --- a/History.md +++ b/History.md @@ -46,6 +46,8 @@ ## ⚡ Performance +* Resolve view paths asynchronously. `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls: the view path is now resolved with asynchronous `fs.stat` calls on the first render (and memoized for subsequent renders), with concurrent stat calls limited to keep the threadpool available under load. Notes for code touching `View` internals: `view.path` is populated on first render instead of at construction, `View.prototype.lookup` is now callback-based, `View.prototype.resolve` was removed, and a custom view class set via `app.set('view', ...)` only needs a `(name, options)` constructor and a `render(options, callback)` method — it no longer needs to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) + * Avoid duplicate Content-Type header processing in `res.send()` when sending string responses without an explicit Content-Type header - by [@bjohansebas](https://github.com/bjohansebas) in [#6991](https://github.com/expressjs/express/pull/6991) 5.2.1 / 2025-12-01 diff --git a/test/app.render.js b/test/app.render.js index db9c761f04c..b94edc5af7f 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -241,8 +241,9 @@ describe('app', function(){ var app = express(); function View(name, options){ + // a custom view only needs a (name, options) constructor + // and a render(options, callback) method this.name = name; - this.path = 'path is required by application.js as a signal of success even though it is not used there.'; } View.prototype.render = function(options, fn){ From 82742aa078a905f35a3cdb791faff2d2c59f8331 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 20:26:17 -0500 Subject: [PATCH 08/19] refactor(view): drop dead error plumbing and derive the lookup file name --- lib/view.js | 81 ++++++++++++++++++++++------------------------ test/app.render.js | 22 ++++++------- 2 files changed, 49 insertions(+), 54 deletions(-) diff --git a/lib/view.js b/lib/view.js index 1457e51b6f2..3a664137c47 100644 --- a/lib/view.js +++ b/lib/view.js @@ -61,15 +61,11 @@ function View(name, options) { throw new Error('No default engine was specified and no extension was provided.'); } - var fileName = name; - if (!this.ext) { // get extension from default engine name this.ext = this.defaultEngine[0] !== '.' ? '.' + this.defaultEngine : this.defaultEngine; - - fileName += this.ext; } if (!opts.engines[this.ext]) { @@ -89,44 +85,46 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; - - // file name to lookup on first render - this.fileName = fileName; } /** - * Lookup view by the given `name` + * Lookup the view file, calling `cb` with the resolved + * path or `null` when not found. * - * @param {string} name * @param {function} cb * @private */ -View.prototype.lookup = function lookup(name, cb) { - var roots = [].concat(this.root); - var ext = this.ext; +View.prototype.lookup = function lookup(cb) { + const roots = [].concat(this.root); + const ext = this.ext; + + // append the default-engine extension unless the name already has one + const name = extname(this.name) + ? this.name + : this.name + ext; debug('lookup "%s"', name); tryNextRoot(); function tryNextRoot() { - var root = roots.shift(); + const root = roots.shift(); if (!root) { - return cb(null, null); + return cb(null); } // resolve the path - var loc = resolve(root, name); + const loc = resolve(root, name); // resolve the file resolveView(dirname(loc), basename(loc), ext, onResolved); } - function onResolved(err, filePath) { - if (err || filePath) { - return cb(err, filePath); + function onResolved(filePath) { + if (filePath) { + return cb(filePath); } // not found; try the next root @@ -146,20 +144,16 @@ View.prototype.lookup = function lookup(name, cb) { */ View.prototype.render = function render(options, callback) { - var view = this; + const view = this; if (this.path) { return renderFile(this, options, callback); } // lookup path on first render - this.lookup(this.fileName, onLookup); - - function onLookup(err, filePath) { - if (err) { - return callback(err); - } + this.lookup(onLookup); + function onLookup(filePath) { if (!filePath) { return callback(lookupFailedError(view)); } @@ -179,7 +173,7 @@ View.prototype.render = function render(options, callback) { */ function renderFile(view, options, callback) { - var sync = true; + let sync = true; debug('render "%s"', view.path); @@ -199,10 +193,10 @@ function renderFile(view, options, callback) { } // copy arguments - var args = new Array(arguments.length); - var cntx = this; + const args = new Array(arguments.length); + const cntx = this; - for (var i = 0; i < arguments.length; i++) { + for (let i = 0; i < arguments.length; i++) { args[i] = arguments[i]; } @@ -222,16 +216,17 @@ function renderFile(view, options, callback) { */ function lookupFailedError(view) { - var dirs = Array.isArray(view.root) && view.root.length > 1 + const dirs = Array.isArray(view.root) && view.root.length > 1 ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' : 'directory "' + view.root + '"' - var err = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); + const err = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); err.view = view; return err; } /** - * Resolve the file within the given directory. + * Resolve the file within the given directory, calling `cb` + * with the resolved path or `null` when not found. * * @param {string} dir * @param {string} file @@ -241,15 +236,15 @@ function lookupFailedError(view) { */ function resolveView(dir, file, ext, cb) { - var filePath = join(dir, file); - var indexPath = join(dir, basename(file, ext), 'index' + ext); + const filePath = join(dir, file); + const indexPath = join(dir, basename(file, ext), 'index' + ext); // . limitStat(filePath, onFileStat); function onFileStat(err, stat) { if (!err && stat.isFile()) { - return cb(null, filePath); + return cb(filePath); } // /index. @@ -258,11 +253,11 @@ function resolveView(dir, file, ext, cb) { function onIndexStat(err, stat) { if (!err && stat.isFile()) { - return cb(null, indexPath); + return cb(indexPath); } // treat any stat error as a miss, like the sync implementation did - cb(null, null); + cb(null); } } @@ -271,9 +266,9 @@ function resolveView(dir, file, ext, cb) { * @private */ -var MAX_PENDING_STATS = 10; -var pendingStats = []; -var numPendingStats = 0; +const MAX_PENDING_STATS = 10; +const pendingStats = []; +let numPendingStats = 0; /** * An fs.stat call that limits the number of outstanding requests. @@ -293,9 +288,9 @@ function statNext() { return; } - var next = pendingStats.shift(); - var path = next[0]; - var cb = next[1]; + const next = pendingStats.shift(); + const path = next[0]; + const cb = next[1]; numPendingStats++; debug('stat "%s"', path); diff --git a/test/app.render.js b/test/app.render.js index b94edc5af7f..63cf882b3cf 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -107,7 +107,7 @@ describe('app', function(){ }) it('should pass a sync engine throw to the callback on first render', function(done){ - var app = express(); + const app = express(); app.engine('tmpl', function () { throw new Error('oops') @@ -201,8 +201,8 @@ describe('app', function(){ }) it('should ignore stat errors and keep looking up', function(done){ - var app = createApp(); - var views = [ + const app = createApp(); + const views = [ path.join(__dirname, 'fixtures', 'user.tmpl'), // a file: lookups inside it fail with ENOTDIR path.join(__dirname, 'fixtures') ] @@ -418,14 +418,14 @@ describe('app', function(){ describe('view lookup concurrency', function () { it('should complete when more renders are in flight than the stat limit', function (done) { - var app = createApp(); + const app = createApp(); app.set('views', path.join(__dirname, 'fixtures')) app.locals.user = { name: 'tobi' }; - var remaining = 30; + let remaining = 30; - for (var i = 0; i < 30; i++) { + for (let i = 0; i < 30; i++) { app.render('user.tmpl', function (err, str) { if (err) return done(err); assert.strictEqual(str, '

tobi

') @@ -436,25 +436,25 @@ describe('app', function(){ describe('when a render callback throws', function () { it('should not stall subsequent lookups', function (done) { - var app = createApp(); + const app = createApp(); app.set('views', path.join(__dirname, 'fixtures')) app.locals.user = { name: 'tobi' }; // stub fs.stat to call back synchronously so the throw below // propagates to this stack frame instead of crashing the process - var realStat = fs.stat; + const realStat = fs.stat; fs.stat = function stubStat(path, cb) { - var err = new Error('stubbed ENOENT'); + const err = new Error('stubbed ENOENT'); err.code = 'ENOENT'; cb(err); }; - var thrown = 0; + let thrown = 0; try { // exhaust the stat limit with callbacks that throw - for (var i = 0; i < 12; i++) { + for (let i = 0; i < 12; i++) { try { app.render('does-not-exist.tmpl', function () { throw new Error('boom'); From 449845d1869e6f18ebeb63eb6bd8fcbc81ec9141 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 20:37:38 -0500 Subject: [PATCH 09/19] perf(view): coalesce concurrent lookups of the same view --- History.md | 2 +- lib/view.js | 27 +++++++++++++++++++++++---- test/app.render.js | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/History.md b/History.md index e37c98fc24d..f87223051c5 100644 --- a/History.md +++ b/History.md @@ -46,7 +46,7 @@ ## ⚡ Performance -* Resolve view paths asynchronously. `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls: the view path is now resolved with asynchronous `fs.stat` calls on the first render (and memoized for subsequent renders), with concurrent stat calls limited to keep the threadpool available under load. Notes for code touching `View` internals: `view.path` is populated on first render instead of at construction, `View.prototype.lookup` is now callback-based, `View.prototype.resolve` was removed, and a custom view class set via `app.set('view', ...)` only needs a `(name, options)` constructor and a `render(options, callback)` method — it no longer needs to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) +* Resolve view paths asynchronously. `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls: the view path is now resolved with asynchronous `fs.stat` calls on the first render (memoized for subsequent renders, and concurrent renders of a not-yet-resolved view share a single lookup), with concurrent stat calls limited to keep the threadpool available under load. Notes for code touching `View` internals: `view.path` is populated on first render instead of at construction, `View.prototype.lookup` is now callback-based, `View.prototype.resolve` was removed, and a custom view class set via `app.set('view', ...)` only needs a `(name, options)` constructor and a `render(options, callback)` method — it no longer needs to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) * Avoid duplicate Content-Type header processing in `res.send()` when sending string responses without an explicit Content-Type header - by [@bjohansebas](https://github.com/bjohansebas) in [#6991](https://github.com/expressjs/express/pull/6991) diff --git a/lib/view.js b/lib/view.js index 3a664137c47..ad5df567395 100644 --- a/lib/view.js +++ b/lib/view.js @@ -85,6 +85,9 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; + + // renders waiting on the in-flight path lookup + this.pendingRenders = null; } /** @@ -150,16 +153,32 @@ View.prototype.render = function render(options, callback) { return renderFile(this, options, callback); } + // coalesce concurrent renders onto the in-flight lookup + if (this.pendingRenders) { + this.pendingRenders.push([options, callback]); + return; + } + + this.pendingRenders = [[options, callback]]; + // lookup path on first render this.lookup(onLookup); function onLookup(filePath) { - if (!filePath) { - return callback(lookupFailedError(view)); + const pending = view.pendingRenders; + view.pendingRenders = null; + + if (filePath) { + view.path = filePath; } - view.path = filePath; - renderFile(view, options, callback); + for (let i = 0; i < pending.length; i++) { + if (filePath) { + renderFile(view, pending[i][0], pending[i][1]); + } else { + pending[i][1](lookupFailedError(view)); + } + } } }; diff --git a/test/app.render.js b/test/app.render.js index 63cf882b3cf..db96eeb0573 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -434,6 +434,43 @@ describe('app', function(){ } }) + it('should coalesce concurrent lookups of the same cached view', function (done) { + const app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.enable('view cache') + app.locals.user = { name: 'tobi' }; + + // count stat calls while delegating to the real fs.stat + const realStat = fs.stat; + let stats = 0; + fs.stat = function countingStat() { + stats++; + return realStat.apply(fs, arguments); + }; + + let remaining = 10; + + for (let i = 0; i < 10; i++) { + app.render('user.tmpl', function (err, str) { + if (remaining === 0) return; // already failed + if (err) { + remaining = 0; + fs.stat = realStat; + return done(err); + } + + assert.strictEqual(str, '

tobi

') + + if (--remaining === 0) { + fs.stat = realStat; + assert.strictEqual(stats, 1) + done(); + } + }) + } + }) + describe('when a render callback throws', function () { it('should not stall subsequent lookups', function (done) { const app = createApp(); From 9de5eb5bd60a4922657ff27b8f60291f42c344a4 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 20:46:44 -0500 Subject: [PATCH 10/19] add test --- test/app.render.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/app.render.js b/test/app.render.js index db96eeb0573..b9337b4d6c3 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -471,6 +471,23 @@ describe('app', function(){ } }) + it('should deliver the lookup error to every coalesced render', function (done) { + const app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.enable('view cache') + + let remaining = 10; + + for (let i = 0; i < 10; i++) { + app.render('does-not-exist.tmpl', function (err, str) { + assert.ok(err) + assert.ok(/^Failed to lookup view/.test(err.message)) + if (--remaining === 0) done(); + }) + } + }) + describe('when a render callback throws', function () { it('should not stall subsequent lookups', function (done) { const app = createApp(); From 8462786954b1ca6701aabfd626d3b20e8aec5ccc Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 21:01:50 -0500 Subject: [PATCH 11/19] fix(view): reset pendingRenders when lookup throws synchronously --- lib/view.js | 12 ++++++++++-- test/app.render.js | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/lib/view.js b/lib/view.js index ad5df567395..313d818d088 100644 --- a/lib/view.js +++ b/lib/view.js @@ -159,10 +159,18 @@ View.prototype.render = function render(options, callback) { return; } - this.pendingRenders = [[options, callback]]; + const queued = this.pendingRenders = [[options, callback]]; // lookup path on first render - this.lookup(onLookup); + try { + this.lookup(onLookup); + } catch (err) { + // a synchronous throw must not leave the queue blocking future renders + if (this.pendingRenders === queued) { + this.pendingRenders = null; + } + throw err; + } function onLookup(filePath) { const pending = view.pendingRenders; diff --git a/test/app.render.js b/test/app.render.js index b9337b4d6c3..120bdd6a00c 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -471,6 +471,23 @@ describe('app', function(){ } }) + it('should not poison the cached view when lookup throws', function (done) { + const app = createApp(); + + app.set('views', 123); // misconfigured: lookup throws synchronously + app.enable('view cache') + + app.render('user.tmpl', function (err) { + assert.ok(err) + + // the cached instance must accept new renders, not queue them forever + app.render('user.tmpl', function (err2) { + assert.ok(err2) + done() + }) + }) + }) + it('should deliver the lookup error to every coalesced render', function (done) { const app = createApp(); From de6eb6627bf42948e7de00e5407a1d1af918d091 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 21:06:35 -0500 Subject: [PATCH 12/19] fix(view): survive synchronous throws from fs.stat and skip falsy view roots --- lib/view.js | 65 +++++++++++++++++++++++++++------------------- test/app.render.js | 40 ++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/lib/view.js b/lib/view.js index 313d818d088..f1d834e706e 100644 --- a/lib/view.js +++ b/lib/view.js @@ -112,17 +112,22 @@ View.prototype.lookup = function lookup(cb) { tryNextRoot(); function tryNextRoot() { - const root = roots.shift(); + while (roots.length > 0) { + const root = roots.shift(); - if (!root) { - return cb(null); - } + // skip empty entries in a views array + if (!root && root !== '') { + continue; + } + + // resolve the path + const loc = resolve(root, name); - // resolve the path - const loc = resolve(root, name); + // resolve the file + return resolveView(dirname(loc), basename(loc), ext, onResolved); + } - // resolve the file - resolveView(dirname(loc), basename(loc), ext, onResolved); + cb(null); } function onResolved(filePath) { @@ -311,24 +316,30 @@ function limitStat(path, cb) { } function statNext() { - if (numPendingStats >= MAX_PENDING_STATS || pendingStats.length === 0) { - return; + while (numPendingStats < MAX_PENDING_STATS && pendingStats.length > 0) { + const next = pendingStats.shift(); + const path = next[0]; + const cb = next[1]; + + numPendingStats++; + debug('stat "%s"', path); + + try { + fs.stat(path, function onStat(err, stat) { + numPendingStats--; + + // dispatch the next queued stat before invoking the callback so a + // throwing callback cannot stall the queue + statNext(); + + cb(err, stat); + }); + return; + } catch (err) { + // fs.stat can throw synchronously (e.g. a path with a null byte); + // release the slot and report it like an asynchronous stat error + numPendingStats--; + cb(err, null); + } } - - const next = pendingStats.shift(); - const path = next[0]; - const cb = next[1]; - - numPendingStats++; - debug('stat "%s"', path); - - fs.stat(path, function onStat(err, stat) { - numPendingStats--; - - // dispatch the next queued stat before invoking the callback so a - // throwing callback cannot stall the queue - statNext(); - - cb(err, stat); - }); } diff --git a/test/app.render.js b/test/app.render.js index 120bdd6a00c..7fde242bcbd 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -200,6 +200,19 @@ describe('app', function(){ }) }) + it('should continue past falsy roots', function(done){ + const app = createApp(); + + app.set('views', ['', null, path.join(__dirname, 'fixtures')]); + app.locals.user = { name: 'tobi' }; + + app.render('user.tmpl', function (err, str) { + if (err) return done(err); + assert.strictEqual(str, '

tobi

') + done(); + }) + }) + it('should ignore stat errors and keep looking up', function(done){ const app = createApp(); const views = [ @@ -471,6 +484,33 @@ describe('app', function(){ } }) + it('should treat a synchronously-throwing stat as a lookup miss', function (done) { + const app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + + // a null byte makes fs.stat throw synchronously instead of + // reporting the error through its callback + const bad = 'nul\u0000byte.tmpl'; + let remaining = 12; + + for (let i = 0; i < 12; i++) { + app.render(bad, function (err) { + assert.ok(err) + assert.ok(/^Failed to lookup view/.test(err.message)) + if (--remaining > 0) return; + + // the stat queue must not have leaked any slots + app.locals.user = { name: 'tobi' }; + app.render('user.tmpl', function (err2, str) { + if (err2) return done(err2); + assert.strictEqual(str, '

tobi

') + done(); + }) + }) + } + }) + it('should not poison the cached view when lookup throws', function (done) { const app = createApp(); From b250f32b786b04f639dc9618ace704a03bd874cd Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 21:13:19 -0500 Subject: [PATCH 13/19] fix(view): guarantee the render callback is invoked at most once --- lib/view.js | 9 ++++++ test/app.render.js | 80 +++++++++++++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 30 deletions(-) diff --git a/lib/view.js b/lib/view.js index f1d834e706e..e1004204355 100644 --- a/lib/view.js +++ b/lib/view.js @@ -86,6 +86,9 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; + // resolved on first render + this.path = null; + // renders waiting on the in-flight path lookup this.pendingRenders = null; } @@ -206,6 +209,7 @@ View.prototype.render = function render(options, callback) { function renderFile(view, options, callback) { let sync = true; + let called = false; debug('render "%s"', view.path); @@ -220,6 +224,11 @@ function renderFile(view, options, callback) { sync = false; function onRender() { + // ignore engines that call back more than once (or throw after + // calling back) + if (called) return; + called = true; + if (!sync) { return callback.apply(this, arguments); } diff --git a/test/app.render.js b/test/app.render.js index 7fde242bcbd..34e65c0558e 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -1,5 +1,6 @@ 'use strict' +var after = require('after') var assert = require('node:assert') var express = require('..'); var fs = require('node:fs') @@ -120,6 +121,29 @@ describe('app', function(){ done() }) }) + + it('should invoke the callback once when the engine calls back and then throws', function(done){ + const app = express(); + + app.engine('tmpl', function (filePath, options, cb) { + cb(null, 'html') + throw new Error('post-callback throw') + }) + app.set('views', path.join(__dirname, 'fixtures')) + + let calls = 0; + + app.render('user.tmpl', function (err, str) { + calls++; + if (err) return done(err); + assert.strictEqual(str, 'html') + + setImmediate(function () { + assert.strictEqual(calls, 1) + done() + }) + }) + }) }) describe('when an extension is given', function(){ @@ -436,13 +460,13 @@ describe('app', function(){ app.set('views', path.join(__dirname, 'fixtures')) app.locals.user = { name: 'tobi' }; - let remaining = 30; + const cb = after(30, done); for (let i = 0; i < 30; i++) { app.render('user.tmpl', function (err, str) { - if (err) return done(err); + if (err) return cb(err); assert.strictEqual(str, '

tobi

') - if (--remaining === 0) done(); + cb(); }) } }) @@ -462,24 +486,18 @@ describe('app', function(){ return realStat.apply(fs, arguments); }; - let remaining = 10; + const cb = after(10, function (err) { + fs.stat = realStat; + if (err) return done(err); + assert.strictEqual(stats, 1) + done(); + }); for (let i = 0; i < 10; i++) { app.render('user.tmpl', function (err, str) { - if (remaining === 0) return; // already failed - if (err) { - remaining = 0; - fs.stat = realStat; - return done(err); - } - + if (err) return cb(err); assert.strictEqual(str, '

tobi

') - - if (--remaining === 0) { - fs.stat = realStat; - assert.strictEqual(stats, 1) - done(); - } + cb(); }) } }) @@ -492,21 +510,23 @@ describe('app', function(){ // a null byte makes fs.stat throw synchronously instead of // reporting the error through its callback const bad = 'nul\u0000byte.tmpl'; - let remaining = 12; + const cb = after(12, function (err) { + if (err) return done(err); + + // the stat queue must not have leaked any slots + app.locals.user = { name: 'tobi' }; + app.render('user.tmpl', function (err2, str) { + if (err2) return done(err2); + assert.strictEqual(str, '

tobi

') + done(); + }) + }); for (let i = 0; i < 12; i++) { app.render(bad, function (err) { assert.ok(err) assert.ok(/^Failed to lookup view/.test(err.message)) - if (--remaining > 0) return; - - // the stat queue must not have leaked any slots - app.locals.user = { name: 'tobi' }; - app.render('user.tmpl', function (err2, str) { - if (err2) return done(err2); - assert.strictEqual(str, '

tobi

') - done(); - }) + cb(); }) } }) @@ -534,13 +554,13 @@ describe('app', function(){ app.set('views', path.join(__dirname, 'fixtures')) app.enable('view cache') - let remaining = 10; + const cb = after(10, done); for (let i = 0; i < 10; i++) { - app.render('does-not-exist.tmpl', function (err, str) { + app.render('does-not-exist.tmpl', function (err) { assert.ok(err) assert.ok(/^Failed to lookup view/.test(err.message)) - if (--remaining === 0) done(); + cb(); }) } }) From b367cdf8756c928379eaae5dd25ed772d6098bd5 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 22:53:57 -0500 Subject: [PATCH 14/19] fix(view): handle synchronous stat failures asynchronously to prevent callback stalls --- History.md | 2 +- lib/view.js | 26 ++++++++++++++------ test/app.render.js | 61 ++++++++++++---------------------------------- 3 files changed, 35 insertions(+), 54 deletions(-) diff --git a/History.md b/History.md index f87223051c5..af869780ee4 100644 --- a/History.md +++ b/History.md @@ -46,7 +46,7 @@ ## ⚡ Performance -* Resolve view paths asynchronously. `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls: the view path is now resolved with asynchronous `fs.stat` calls on the first render (memoized for subsequent renders, and concurrent renders of a not-yet-resolved view share a single lookup), with concurrent stat calls limited to keep the threadpool available under load. Notes for code touching `View` internals: `view.path` is populated on first render instead of at construction, `View.prototype.lookup` is now callback-based, `View.prototype.resolve` was removed, and a custom view class set via `app.set('view', ...)` only needs a `(name, options)` constructor and a `render(options, callback)` method — it no longer needs to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) +* Resolve view paths asynchronously, so `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls. The render callback is now always invoked asynchronously, `view.path` is resolved on first render instead of at construction, `View.prototype.lookup` is callback-based, `View.prototype.resolve` was removed, and custom view classes no longer need to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) * Avoid duplicate Content-Type header processing in `res.send()` when sending string responses without an explicit Content-Type header - by [@bjohansebas](https://github.com/bjohansebas) in [#6991](https://github.com/expressjs/express/pull/6991) diff --git a/lib/view.js b/lib/view.js index e1004204355..172814a12d8 100644 --- a/lib/view.js +++ b/lib/view.js @@ -167,16 +167,15 @@ View.prototype.render = function render(options, callback) { return; } - const queued = this.pendingRenders = [[options, callback]]; + this.pendingRenders = [[options, callback]]; // lookup path on first render try { this.lookup(onLookup); } catch (err) { - // a synchronous throw must not leave the queue blocking future renders - if (this.pendingRenders === queued) { - this.pendingRenders = null; - } + // a synchronous throw happens before any stat is dispatched, so the + // queue is still ours; reset it so future renders are not blocked + this.pendingRenders = null; throw err; } @@ -192,7 +191,9 @@ View.prototype.render = function render(options, callback) { if (filePath) { renderFile(view, pending[i][0], pending[i][1]); } else { - pending[i][1](lookupFailedError(view)); + // deliver asynchronously so one throwing callback cannot skip + // the remaining coalesced renders + process.nextTick(pending[i][1], lookupFailedError(view)); } } } @@ -324,6 +325,14 @@ function limitStat(path, cb) { statNext(); } +/** + * Drain the pending stat queue while under the concurrency limit, + * always dispatching further work before invoking a callback so a + * throwing callback cannot stall the queue. + * + * @private + */ + function statNext() { while (numPendingStats < MAX_PENDING_STATS && pendingStats.length > 0) { const next = pendingStats.shift(); @@ -346,9 +355,10 @@ function statNext() { return; } catch (err) { // fs.stat can throw synchronously (e.g. a path with a null byte); - // release the slot and report it like an asynchronous stat error + // release the slot and deliver the error asynchronously like a real + // stat failure, then keep draining the queue numPendingStats--; - cb(err, null); + process.nextTick(cb, err, null); } } } diff --git a/test/app.render.js b/test/app.render.js index 34e65c0558e..a5a7adeaefb 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -531,6 +531,22 @@ describe('app', function(){ } }) + it('should deliver sync stat failures asynchronously', function (done) { + const app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + + let returned = false; + + app.render('nul\u0000byte.tmpl', function (err) { + assert.ok(returned, 'callback ran synchronously inside app.render') + assert.ok(err) + done() + }) + + returned = true; + }) + it('should not poison the cached view when lookup throws', function (done) { const app = createApp(); @@ -565,51 +581,6 @@ describe('app', function(){ } }) - describe('when a render callback throws', function () { - it('should not stall subsequent lookups', function (done) { - const app = createApp(); - - app.set('views', path.join(__dirname, 'fixtures')) - app.locals.user = { name: 'tobi' }; - - // stub fs.stat to call back synchronously so the throw below - // propagates to this stack frame instead of crashing the process - const realStat = fs.stat; - fs.stat = function stubStat(path, cb) { - const err = new Error('stubbed ENOENT'); - err.code = 'ENOENT'; - cb(err); - }; - - let thrown = 0; - - try { - // exhaust the stat limit with callbacks that throw - for (let i = 0; i < 12; i++) { - try { - app.render('does-not-exist.tmpl', function () { - throw new Error('boom'); - }) - } catch (err) { - assert.strictEqual(err.message, 'boom') - thrown++; - } - } - } finally { - fs.stat = realStat; - } - - // every callback must have run despite the throws - assert.strictEqual(thrown, 12) - - // a later render must still complete - app.render('user.tmpl', function (err, str) { - if (err) return done(err); - assert.strictEqual(str, '

tobi

') - done(); - }) - }) - }) }) }) From 29a2d83c9440af27930a626082ecaaa7b6939625 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 23:02:45 -0500 Subject: [PATCH 15/19] test: restore fs.stat via afterEach and simplify falsy-root check --- lib/view.js | 4 ++-- test/app.render.js | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/view.js b/lib/view.js index 172814a12d8..6b6721dc723 100644 --- a/lib/view.js +++ b/lib/view.js @@ -118,8 +118,8 @@ View.prototype.lookup = function lookup(cb) { while (roots.length > 0) { const root = roots.shift(); - // skip empty entries in a views array - if (!root && root !== '') { + // skip missing entries in a views array + if (root == null) { continue; } diff --git a/test/app.render.js b/test/app.render.js index a5a7adeaefb..d1554eabbb9 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -454,6 +454,14 @@ describe('app', function(){ }) describe('view lookup concurrency', function () { + const originalStat = fs.stat; + + afterEach(function () { + // safety net: some tests below patch fs.stat and restore it + // themselves, but a failed assertion must not leak the patch + fs.stat = originalStat; + }) + it('should complete when more renders are in flight than the stat limit', function (done) { const app = createApp(); @@ -564,6 +572,37 @@ describe('app', function(){ }) }) + it('should reuse the resolved path on subsequent renders', function (done) { + const app = createApp(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.enable('view cache') + app.locals.user = { name: 'tobi' }; + + app.render('user.tmpl', function (err, str) { + if (err) return done(err); + assert.strictEqual(str, '

tobi

') + + // count stat calls while delegating to the real fs.stat + const realStat = fs.stat; + let stats = 0; + fs.stat = function countingStat() { + stats++; + return realStat.apply(fs, arguments); + }; + + app.render('user.tmpl', function (err2, str2) { + fs.stat = realStat; + if (err2) return done(err2); + assert.strictEqual(str2, '

tobi

') + + // the memoized path must not be looked up again + assert.strictEqual(stats, 0) + done(); + }) + }) + }) + it('should deliver the lookup error to every coalesced render', function (done) { const app = createApp(); From df777dc9affdf9ac404134b136ffd52803713d9d Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 23:23:05 -0500 Subject: [PATCH 16/19] test: assert sync-parity error messages for misconfigured views --- lib/view.js | 46 ++++++++++++++----------------- test/app.render.js | 67 +++++++++++++++++++++++++++++----------------- 2 files changed, 63 insertions(+), 50 deletions(-) diff --git a/lib/view.js b/lib/view.js index 6b6721dc723..12175d04407 100644 --- a/lib/view.js +++ b/lib/view.js @@ -94,8 +94,8 @@ function View(name, options) { } /** - * Lookup the view file, calling `cb` with the resolved - * path or `null` when not found. + * Lookup the view file, calling `cb` with `(err, filePath)` where + * `filePath` is `null` when the view was not found. * * @param {function} cb * @private @@ -115,27 +115,29 @@ View.prototype.lookup = function lookup(cb) { tryNextRoot(); function tryNextRoot() { - while (roots.length > 0) { - const root = roots.shift(); - - // skip missing entries in a views array - if (root == null) { - continue; - } + if (roots.length === 0) { + return cb(null, null); + } - // resolve the path - const loc = resolve(root, name); + const root = roots.shift(); - // resolve the file - return resolveView(dirname(loc), basename(loc), ext, onResolved); + // resolve the path + let loc; + try { + loc = resolve(root, name); + } catch (err) { + // an invalid entry in the views setting throws the same error + // the synchronous implementation did + return cb(err); } - cb(null); + // resolve the file + resolveView(dirname(loc), basename(loc), ext, onResolved); } function onResolved(filePath) { if (filePath) { - return cb(filePath); + return cb(null, filePath); } // not found; try the next root @@ -170,16 +172,9 @@ View.prototype.render = function render(options, callback) { this.pendingRenders = [[options, callback]]; // lookup path on first render - try { - this.lookup(onLookup); - } catch (err) { - // a synchronous throw happens before any stat is dispatched, so the - // queue is still ours; reset it so future renders are not blocked - this.pendingRenders = null; - throw err; - } + this.lookup(onLookup); - function onLookup(filePath) { + function onLookup(err, filePath) { const pending = view.pendingRenders; view.pendingRenders = null; @@ -193,7 +188,7 @@ View.prototype.render = function render(options, callback) { } else { // deliver asynchronously so one throwing callback cannot skip // the remaining coalesced renders - process.nextTick(pending[i][1], lookupFailedError(view)); + process.nextTick(pending[i][1], err || lookupFailedError(view)); } } } @@ -352,7 +347,6 @@ function statNext() { cb(err, stat); }); - return; } catch (err) { // fs.stat can throw synchronously (e.g. a path with a null byte); // release the slot and deliver the error asynchronously like a real diff --git a/test/app.render.js b/test/app.render.js index d1554eabbb9..7c66879b7c5 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -224,10 +224,10 @@ describe('app', function(){ }) }) - it('should continue past falsy roots', function(done){ + it('should continue past an empty-string root', function(done){ const app = createApp(); - app.set('views', ['', null, path.join(__dirname, 'fixtures')]); + app.set('views', ['', path.join(__dirname, 'fixtures')]); app.locals.user = { name: 'tobi' }; app.render('user.tmpl', function (err, str) { @@ -462,6 +462,22 @@ describe('app', function(){ fs.stat = originalStat; }) + // count fs.stat calls while delegating to the real implementation + function countStats() { + const realStat = fs.stat; + const counter = { + n: 0, + restore: function () { fs.stat = realStat; } + }; + + fs.stat = function countingStat() { + counter.n++; + return realStat.apply(fs, arguments); + }; + + return counter; + } + it('should complete when more renders are in flight than the stat limit', function (done) { const app = createApp(); @@ -486,18 +502,12 @@ describe('app', function(){ app.enable('view cache') app.locals.user = { name: 'tobi' }; - // count stat calls while delegating to the real fs.stat - const realStat = fs.stat; - let stats = 0; - fs.stat = function countingStat() { - stats++; - return realStat.apply(fs, arguments); - }; + const stats = countStats(); const cb = after(10, function (err) { - fs.stat = realStat; + stats.restore(); if (err) return done(err); - assert.strictEqual(stats, 1) + assert.strictEqual(stats.n, 1) done(); }); @@ -555,23 +565,38 @@ describe('app', function(){ returned = true; }) - it('should not poison the cached view when lookup throws', function (done) { + it('should error like the sync implementation for a misconfigured views setting', function (done) { const app = createApp(); - app.set('views', 123); // misconfigured: lookup throws synchronously + app.set('views', 123); // misconfigured: not a string app.enable('view cache') app.render('user.tmpl', function (err) { - assert.ok(err) + assert.ok(err instanceof TypeError) + assert.ok(/must be of type string/.test(err.message)) // the cached instance must accept new renders, not queue them forever app.render('user.tmpl', function (err2) { - assert.ok(err2) + assert.ok(err2 instanceof TypeError) + assert.ok(/must be of type string/.test(err2.message)) done() }) }) }) + it('should error instead of crashing when a later root is invalid', function (done) { + const app = createApp(); + + app.set('views', [path.join(__dirname, 'fixtures', 'local_layout'), 123]); + + // email.tmpl does not exist in the first (valid) root + app.render('email.tmpl', function (err) { + assert.ok(err instanceof TypeError) + assert.ok(/must be of type string/.test(err.message)) + done() + }) + }) + it('should reuse the resolved path on subsequent renders', function (done) { const app = createApp(); @@ -583,21 +608,15 @@ describe('app', function(){ if (err) return done(err); assert.strictEqual(str, '

tobi

') - // count stat calls while delegating to the real fs.stat - const realStat = fs.stat; - let stats = 0; - fs.stat = function countingStat() { - stats++; - return realStat.apply(fs, arguments); - }; + const stats = countStats(); app.render('user.tmpl', function (err2, str2) { - fs.stat = realStat; + stats.restore(); if (err2) return done(err2); assert.strictEqual(str2, '

tobi

') // the memoized path must not be looked up again - assert.strictEqual(stats, 0) + assert.strictEqual(stats.n, 0) done(); }) }) From 97ba3e452fd1e5f66b39731122b17e7ec4a620a1 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 23:32:30 -0500 Subject: [PATCH 17/19] fix(app): keep requiring path from custom view classes --- History.md | 2 +- lib/application.js | 15 +++++++++++++-- lib/view.js | 2 +- test/app.render.js | 24 ++++++++++++++++++++++-- 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/History.md b/History.md index af869780ee4..a6a4c8635db 100644 --- a/History.md +++ b/History.md @@ -46,7 +46,7 @@ ## ⚡ Performance -* Resolve view paths asynchronously, so `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls. The render callback is now always invoked asynchronously, `view.path` is resolved on first render instead of at construction, `View.prototype.lookup` is callback-based, `View.prototype.resolve` was removed, and custom view classes no longer need to set `this.path` - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) +* Resolve view paths asynchronously, so `res.render()` and `app.render()` no longer block the event loop with synchronous filesystem calls. The render callback is now always invoked asynchronously, `view.path` is resolved on first render instead of at construction, `View.prototype.lookup` is callback-based, and `View.prototype.resolve` was removed - by [@aredridel](https://github.com/aredridel) and [@bjohansebas](https://github.com/bjohansebas) in [#2653](https://github.com/expressjs/express/pull/2653) * Avoid duplicate Content-Type header processing in `res.send()` when sending string responses without an explicit Content-Type header - by [@bjohansebas](https://github.com/bjohansebas) in [#6991](https://github.com/expressjs/express/pull/6991) diff --git a/lib/application.js b/lib/application.js index cd46ad944d5..25169cefea9 100644 --- a/lib/application.js +++ b/lib/application.js @@ -547,14 +547,25 @@ app.render = function render(name, options, callback) { // view if (!view) { - var View = this.get('view'); + var ViewClass = this.get('view'); - view = new View(name, { + view = new ViewClass(name, { defaultEngine: this.get('view engine'), root: this.get('views'), engines: engines }); + // custom view classes keep the classic contract where a missing + // `path` signals a failed lookup; the built-in View resolves lazily + if (!(view instanceof View) && !view.path) { + var dirs = Array.isArray(view.root) && view.root.length > 1 + ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' + : 'directory "' + view.root + '"'; + var err = new Error('Failed to lookup view "' + name + '" in views ' + dirs); + err.view = view; + return done(err); + } + // prime the cache if (renderOptions.cache) { cache[name] = view; diff --git a/lib/view.js b/lib/view.js index 12175d04407..767b16c4e35 100644 --- a/lib/view.js +++ b/lib/view.js @@ -255,7 +255,7 @@ function renderFile(view, options, callback) { function lookupFailedError(view) { const dirs = Array.isArray(view.root) && view.root.length > 1 ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' - : 'directory "' + view.root + '"' + : 'directory "' + view.root + '"'; const err = new Error('Failed to lookup view "' + view.name + '" in views ' + dirs); err.view = view; return err; diff --git a/test/app.render.js b/test/app.render.js index 7c66879b7c5..a0cd7ee3b5e 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -278,9 +278,8 @@ describe('app', function(){ var app = express(); function View(name, options){ - // a custom view only needs a (name, options) constructor - // and a render(options, callback) method this.name = name; + this.path = 'path is required as a signal of success for custom view classes.'; } View.prototype.render = function(options, fn){ @@ -295,6 +294,27 @@ describe('app', function(){ done(); }) }) + + it('should error when a custom view leaves path unset', function(done){ + var app = express(); + + function View(name, options){ + this.name = name; + this.root = options.root; + } + + View.prototype.render = function(){ + throw new Error('render should not be called') + }; + + app.set('view', View); + + app.render('something', function(err){ + assert.ok(err) + assert.ok(/^Failed to lookup view "something"/.test(err.message)) + done(); + }) + }) }) describe('caching', function(){ From 38278a671505ed1545f720b40433be04efd37b7b Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Sun, 12 Jul 2026 23:38:37 -0500 Subject: [PATCH 18/19] fix(app): drop views with failed lookups from the cache like the sync implementation --- lib/application.js | 9 +++++++++ test/app.render.js | 22 ++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/lib/application.js b/lib/application.js index 25169cefea9..af13558e3ce 100644 --- a/lib/application.js +++ b/lib/application.js @@ -569,6 +569,15 @@ app.render = function render(name, options, callback) { // prime the cache if (renderOptions.cache) { cache[name] = view; + + var callerDone = done; + done = function (err, str) { + // match the sync behavior: a view whose lookup failed is not kept + if (err && !view.path) { + delete cache[name]; + } + callerDone(err, str); + }; } } diff --git a/test/app.render.js b/test/app.render.js index a0cd7ee3b5e..cc3da5e1acc 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -617,6 +617,28 @@ describe('app', function(){ }) }) + it('should not cache views whose lookup failed', function (done) { + const app = createApp(); + + app.enable('view cache') + app.set('views', path.join(__dirname, 'fixtures', 'local_layout')); // email.tmpl is not here + + app.render('email.tmpl', function (err) { + assert.ok(err) + assert.ok(/^Failed to lookup view/.test(err.message)) + + // like the sync implementation, the failed view must not stick: + // a new render honors the current settings + app.set('views', path.join(__dirname, 'fixtures')); + + app.render('email.tmpl', function (err2, str) { + if (err2) return done(err2); + assert.strictEqual(str, '

This is an email

') + done(); + }) + }) + }) + it('should reuse the resolved path on subsequent renders', function (done) { const app = createApp(); From 4859afa6c2a5ffbd7de850e036915823b6f79d97 Mon Sep 17 00:00:00 2001 From: Sebastian Beltran Date: Mon, 13 Jul 2026 00:05:16 -0500 Subject: [PATCH 19/19] fix(view): clarify path resolution behavior for custom view subclasses --- lib/application.js | 7 ++++--- test/app.render.js | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/application.js b/lib/application.js index af13558e3ce..bc2da5fea4f 100644 --- a/lib/application.js +++ b/lib/application.js @@ -555,9 +555,10 @@ app.render = function render(name, options, callback) { engines: engines }); - // custom view classes keep the classic contract where a missing - // `path` signals a failed lookup; the built-in View resolves lazily - if (!(view instanceof View) && !view.path) { + // only the built-in View resolves its path lazily; any other view + // class (custom or a subclass) keeps the classic contract where a + // missing `path` after construction signals a failed lookup + if (view.constructor !== View && !view.path) { var dirs = Array.isArray(view.root) && view.root.length > 1 ? 'directories "' + view.root.slice(0, -1).join('", "') + '" or "' + view.root[view.root.length - 1] + '"' : 'directory "' + view.root + '"'; diff --git a/test/app.render.js b/test/app.render.js index cc3da5e1acc..1063ece9f01 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -315,6 +315,32 @@ describe('app', function(){ done(); }) }) + + it('should honor a View subclass that clears path to deny a view', function(done){ + var app = express(); + var BaseView = require('../lib/view'); + + app.engine('tmpl', function (p, o, fn) { fn(null, 'served'); }); + app.set('views', path.join(__dirname, 'fixtures')) + + // a subclass may deny a view by leaving path falsy; the inherited + // lazy lookup must not resolve it behind the subclass's back + function DenyView(name, options){ + BaseView.call(this, name, options); + this.path = null; + } + DenyView.prototype = Object.create(BaseView.prototype); + DenyView.prototype.constructor = DenyView; + + app.set('view', DenyView); + + // user.tmpl exists on disk, but the subclass denies it + app.render('user.tmpl', function (err, str){ + assert.ok(err, 'denied view must not be served') + assert.ok(/^Failed to lookup view/.test(err.message)) + done(); + }) + }) }) describe('caching', function(){