diff --git a/History.md b/History.md index 8a4293ab547..a6a4c8635db 100644 --- a/History.md +++ b/History.md @@ -46,6 +46,8 @@ ## ⚡ 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, 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) 5.2.1 / 2025-12-01 diff --git a/lib/application.js b/lib/application.js index 310e6dfef21..bc2da5fea4f 100644 --- a/lib/application.js +++ b/lib/application.js @@ -547,18 +547,21 @@ 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 }); - if (!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 + '"' + : 'directory "' + view.root + '"'; var err = new Error('Failed to lookup view "' + name + '" in views ' + dirs); err.view = view; return done(err); @@ -567,6 +570,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/lib/view.js b/lib/view.js index d66b4a2d89c..767b16c4e35 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]) { @@ -90,62 +86,154 @@ function View(name, options) { // store loaded engine this.engine = opts.engines[this.ext]; - // lookup path - this.path = this.lookup(fileName); + // resolved on first render + this.path = null; + + // renders waiting on the in-flight path lookup + this.pendingRenders = null; } /** - * Lookup view by the given `name` + * Lookup the view file, calling `cb` with `(err, filePath)` where + * `filePath` is `null` when the view was not found. * - * @param {string} name + * @param {function} cb * @private */ -View.prototype.lookup = function lookup(name) { - var path; - var roots = [].concat(this.root); +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); - for (var i = 0; i < roots.length && !path; i++) { - var root = roots[i]; + tryNextRoot(); + + function tryNextRoot() { + if (roots.length === 0) { + return cb(null, null); + } + + const root = roots.shift(); // resolve the path - var loc = resolve(root, name); - var dir = dirname(loc); - var file = basename(loc); + 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); + } // resolve the file - path = this.resolve(dir, file); + resolveView(dirname(loc), basename(loc), ext, onResolved); } - return path; + function onResolved(filePath) { + if (filePath) { + return cb(null, 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; + const view = this; + + if (this.path) { + 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(err, filePath) { + const pending = view.pendingRenders; + view.pendingRenders = null; + + if (filePath) { + view.path = filePath; + } + + for (let i = 0; i < pending.length; i++) { + if (filePath) { + renderFile(view, pending[i][0], pending[i][1]); + } else { + // deliver asynchronously so one throwing callback cannot skip + // the remaining coalesced renders + process.nextTick(pending[i][1], err || lookupFailedError(view)); + } + } + } +}; + +/** + * Render the resolved view path with the given options. + * + * @param {View} view + * @param {object} options + * @param {function} callback + * @private + */ + +function renderFile(view, options, callback) { + let sync = true; + let called = false; - debug('render "%s"', this.path); + debug('render "%s"', view.path); + + 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() { + // ignore engines that call back more than once (or throw after + // calling back) + if (called) return; + called = true; - // render, normalizing sync callbacks - this.engine(this.path, options, function onRender() { if (!sync) { return callback.apply(this, arguments); } // 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]; } @@ -153,53 +241,118 @@ View.prototype.render = function render(options, callback) { return process.nextTick(function renderTick() { return callback.apply(cntx, args); }); - }); + } +} - sync = false; -}; +/** + * Build the error for a failed view lookup. + * + * @param {View} view + * @return {Error} + * @private + */ + +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 + '"'; + 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 + * @param {string} ext + * @param {function} cb * @private */ -View.prototype.resolve = function resolve(dir, file) { - var ext = this.ext; +function resolveView(dir, file, ext, cb) { + const filePath = join(dir, file); + const indexPath = join(dir, basename(file, ext), 'index' + ext); // . - var path = join(dir, file); - var stat = tryStat(path); + limitStat(filePath, onFileStat); + + function onFileStat(err, stat) { + if (!err && stat.isFile()) { + return cb(filePath); + } - if (stat && stat.isFile()) { - return path; + // /index. + limitStat(indexPath, onIndexStat); } - // /index. - path = join(dir, basename(file, ext), 'index' + ext); - stat = tryStat(path); + function onIndexStat(err, stat) { + if (!err && stat.isFile()) { + return cb(indexPath); + } - if (stat && stat.isFile()) { - return path; + // treat any stat error as a miss, like the sync implementation did + cb(null); } -}; +} + +/** + * Module variables for stat concurrency limiting. + * @private + */ + +const MAX_PENDING_STATS = 10; +const pendingStats = []; +let numPendingStats = 0; /** - * Return a stat, maybe. + * An fs.stat call that limits the number of outstanding requests. * * @param {string} path - * @return {fs.Stats} + * @param {function} cb * @private */ -function tryStat(path) { - debug('stat "%s"', path); +function limitStat(path, cb) { + pendingStats.push([path, cb]); + statNext(); +} - try { - return fs.statSync(path); - } catch (e) { - return undefined; +/** + * 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(); + 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); + }); + } 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 + // stat failure, then keep draining the queue + numPendingStats--; + process.nextTick(cb, err, null); + } } } diff --git a/test/app.render.js b/test/app.render.js index bd65ce1035b..1063ece9f01 100644 --- a/test/app.render.js +++ b/test/app.render.js @@ -1,7 +1,9 @@ 'use strict' +var after = require('after') var assert = require('node:assert') var express = require('..'); +var fs = require('node:fs') var path = require('node:path') var tmpl = require('./support/tmpl'); @@ -104,6 +106,44 @@ describe('app', function(){ done() }) }) + + it('should pass a sync engine throw to the callback on first render', function(done){ + const 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() + }) + }) + + 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(){ @@ -184,6 +224,36 @@ describe('app', function(){ }) }) + it('should continue past an empty-string root', function(done){ + const app = createApp(); + + app.set('views', ['', 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 = [ + 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 = [ @@ -209,7 +279,7 @@ describe('app', function(){ function View(name, options){ this.name = name; - this.path = 'path is required by application.js as a signal of success even though it is not used there.'; + this.path = 'path is required as a signal of success for custom view classes.'; } View.prototype.render = function(options, fn){ @@ -224,6 +294,53 @@ 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(); + }) + }) + + 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(){ @@ -381,6 +498,216 @@ 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; + }) + + // 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(); + + app.set('views', path.join(__dirname, 'fixtures')) + app.locals.user = { name: 'tobi' }; + + const cb = after(30, done); + + for (let i = 0; i < 30; i++) { + app.render('user.tmpl', function (err, str) { + if (err) return cb(err); + assert.strictEqual(str, '

tobi

') + cb(); + }) + } + }) + + 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' }; + + const stats = countStats(); + + const cb = after(10, function (err) { + stats.restore(); + if (err) return done(err); + assert.strictEqual(stats.n, 1) + done(); + }); + + for (let i = 0; i < 10; i++) { + app.render('user.tmpl', function (err, str) { + if (err) return cb(err); + assert.strictEqual(str, '

tobi

') + cb(); + }) + } + }) + + 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'; + 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)) + cb(); + }) + } + }) + + 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 error like the sync implementation for a misconfigured views setting', function (done) { + const app = createApp(); + + app.set('views', 123); // misconfigured: not a string + app.enable('view cache') + + app.render('user.tmpl', function (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 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 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(); + + 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

') + + const stats = countStats(); + + app.render('user.tmpl', function (err2, str2) { + stats.restore(); + if (err2) return done(err2); + assert.strictEqual(str2, '

tobi

') + + // the memoized path must not be looked up again + assert.strictEqual(stats.n, 0) + done(); + }) + }) + }) + + 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') + + const cb = after(10, done); + + for (let i = 0; i < 10; i++) { + app.render('does-not-exist.tmpl', function (err) { + assert.ok(err) + assert.ok(/^Failed to lookup view/.test(err.message)) + cb(); + }) + } + }) + + }) }) function createApp() {