express: ensure that apps are at the top of stack when 'mount' is emi…

…tted

lib/instrumentation/express.js

@@ -8,7 +8,7 @@ var path = require('path')
  , record = require('../metrics/recorders/generic.js')
  , NAMES = require('../metrics/names.js')
  , VIEW = NAMES.VIEW
- 
+
 
 var ORIGINAL = '__NR_original'
 var RESERVED = [ // http://es5.github.io/#x7.6.1.2
@@ -50,7 +50,7 @@ function nameFromRoute(segment, route, params) {
 
  var transaction = segment.trace.transaction
  , path = route.path || route.regexp
- 
+
 
  if (!path) return logger.debug({route : route}, "No path found on Express route.")
 
@@ -110,7 +110,7 @@ module.exports = function initialize(agent, express) {
  var name = VIEW.PREFIX + view + VIEW.RENDER
  , segment = tracer.addSegment(name, record)
  , wrapped
- 
+
 
  if ('function' === typeof options) {
  cb = options
@@ -203,7 +203,7 @@ module.exports = function initialize(agent, express) {
  function wrapHandle(__NR_handle) {
  var arglist
  , name = ''
- 
+
 
  // reiterated: testing function arity is stupid
  switch (__NR_handle.length) {
@@ -229,7 +229,7 @@ module.exports = function initialize(agent, express) {
  var template = function () {
  var args = tracer.slice(arguments)
  , last = args.length - 1
- 
+
 
  if (typeof args[last] === 'function') {
  args[last] = tracer.callbackProxy(args[last])
@@ -253,15 +253,8 @@ module.exports = function initialize(agent, express) {
 
  function wrapMiddlewareStack(route, use) {
  return function cls_wrapMiddlewareStack() {
- if (this.stack && this.stack.length) {
- // Remove our custom error handler.
- this.stack = this.stack.filter(function cb_filter(m) { return m !== interceptor; })
- }
- if (!interceptor) {
- // call use to create a Layer object, then pop it off and store it.
- use.call(this, '/', sentinel)
- interceptor = this.stack.pop()
- }
+ // Remove our custom error handler.
+ removeInterceptor(this)
 
  /* We allow `use` to go through the arguments so it can reject bad things
  * for us so we don't have to also do argument type checking.
@@ -274,7 +267,7 @@ module.exports = function initialize(agent, express) {
  */
  if (!route) {
  // wrap most recently added unwrapped handler
- var top = this.stack[this.stack.length-1]
+ var top = this.stack[this.stack.length - 1]
  if (top) {
  if (top.handle &&
  typeof top.handle === 'function' &&
@@ -284,34 +277,95 @@ module.exports = function initialize(agent, express) {
  }
  }
 
- /* Give the error tracer a better chance of intercepting errors by
- * putting it before the first error handler (a middleware that takes 4
- * parameters, in express's world). Error handlers tend to be placed
- * towards the end of the middleware chain and sometimes don't pass
- * errors along. Don't just put the interceptor at the beginning because
- * we want to allow as many middleware functions to execute as possible
- * before the interceptor is run, to increase error coverage.
- *
- * NOTE: This is heuristic, and works because interceptor propagates
- * errors instead of terminating the middleware chain.
- * Ignores routes.
- */
- var spliced = false
- for (var i = 0; i < this.stack.length; i++) {
- var middleware = this.stack[i]
- // Check to see if it is an error handler middleware
- if (middleware &&
- middleware.handle &&
- middleware.handle.length === 4) {
- this.stack.splice(i, 0, interceptor)
- spliced = true
- break
+ if (!interceptor) {
+ // call use to create a Layer object, then pop it off and store it.
+ use.call(this, '/', sentinel)
+ interceptor = this.stack.pop()
+ }
+
+ addInterceptor(this)
+
+ return app
+ }
+ }
+
+ function wrapAppUse(use) {
+ return function wrappedAppUse() {
+ var emits = []
+ var arg
+
+ // loop over middlewares being used
+ for(var i = 0, l = arguments.length; i < l; ++i) {
+ arg = arguments[i]
+ // if the middleware is an express app
+ if(typeof arg === 'function' && arg.set && arg.handle && arg.emit) {
+ emits[i] = arg.emit
+ // patch emit so it removes the error interceptor since it tends to be
+ // at the top of the stack. when you use another express app, a `mount`
+ // event is fired on the app. It should be at the top of the stack
+ // when this event is fired
+ patchEmit(this, arg)
+ }
+ }
+
+ use.apply(this, arguments)
+
+ // restore orignal emit methods
+ for(var i = 0, l = emits.length; i < l; ++i) {
+ if(emits[i]) {
+ arguments[i] = emits[i]
  }
  }
- if (!spliced) this.stack.push(interceptor)
 
  // don't break chaining
- return app
+ return this
+ }
+ }
+
+ function addInterceptor(app) {
+ /* Give the error tracer a better chance of intercepting errors by
+ * putting it before the first error handler (a middleware that takes 4
+ * parameters, in express's world). Error handlers tend to be placed
+ * towards the end of the middleware chain and sometimes don't pass
+ * errors along. Don't just put the interceptor at the beginning because
+ * we want to allow as many middleware functions to execute as possible
+ * before the interceptor is run, to increase error coverage.
+ *
+ * NOTE: This is heuristic, and works because interceptor propagates
+ * errors instead of terminating the middleware chain.
+ * Ignores routes.
+ */
+ var spliced = false
+ for (var i = 0; i < app.stack.length; i++) {
+ var middleware = app.stack[i]
+ // Check to see if it is an error handler middleware
+ if (middleware &&
+ middleware.handle &&
+ middleware.handle.length === 4) {
+ app.stack.splice(i, 0, interceptor)
+ spliced = true
+ break
+ }
+ }
+ if (!spliced) app.stack.push(interceptor)
+ }
+
+ function removeInterceptor(app) {
+ if (app.stack && app.stack.length) {
+ // Remove our custom error handler.
+ app.stack = app.stack.filter(function cb_filter(m) { return m !== interceptor })
+ }
+ }
+
+ function patchEmit(parent, app) {
+ var emit = app.emit
+ app.emit = patchedEmit
+
+ function patchedEmit() {
+ removeInterceptor(parent._router)
+ var result = emit.apply(app, arguments)
+ addInterceptor(parent._router)
+ return result
  }
  }
 
@@ -345,7 +399,7 @@ module.exports = function initialize(agent, express) {
  */
  var oneoff = express.createServer()
  , Router = oneoff.routes.constructor
- 
+
 
  shimmer.wrapMethod(express,
  'express',
@@ -410,6 +464,11 @@ module.exports = function initialize(agent, express) {
  'express.Router',
  'route',
  wrapMiddlewareStack.bind(null, true))
+
+ shimmer.wrapMethod(express.application,
+ 'express.application',
+ 'use',
+ wrapAppUse)
  break
  default:
  logger.warn("Unrecognized version %d of Express detected; not instrumenting",

test/versioned/express4-ejs/app-use.tap.js

@@ -0,0 +1,34 @@
+'use strict'
+
+var test = require('tap').test
+var helper = require('../../lib/agent_helper')
+
+test('app should be at top of stack when mounted', function (t) {
+ var agent = helper.instrumentMockedAgent()
+ var express = require('express')
+
+ this.tearDown(function cb_tearDown() {
+ helper.unloadAgent(agent)
+ })
+
+ t.plan(2)
+
+ var main = express()
+ var child = express()
+
+ child.on('mount', function() {
+ t.equal(
+ main._router.stack.length,
+ 3,
+ '3 middleware functions: query parser, Express, child'
+ )
+ })
+
+ main.use(child)
+
+ t.equal(
+ main._router.stack.length,
+ 4,
+ '4 middleware functions: query parser, Express, child, error trapper'
+ )
+})

test/versioned/express4-ejs/express4-render-ejs.tap.js

@@ -8,7 +8,7 @@ var path = require('path')
  , request = require('request')
  , helper = require('../../lib/agent_helper')
  , API = require('../../../api.js')
- 
+
 
 var TEST_PATH = '/test'
  , TEST_PORT = 9876
@@ -24,7 +24,7 @@ var TEST_PATH = '/test'
  " <p>I heard u like HTML.</p>\n" +
  "</body>\n" +
  "</html>\n"
- 
+
 
 // Regression test for issue 154
 // https://github.com/newrelic/node-newrelic/pull/154
@@ -82,7 +82,7 @@ test("agent instrumentation of Express 4", function (t) {
  var agent = helper.instrumentMockedAgent()
  , app = require('express')()
  , server = require('http').createServer(app)
- 
+
 
  this.tearDown(function cb_tearDown() {
  server.close()
@@ -139,7 +139,7 @@ test("agent instrumentation of Express 4", function (t) {
  var agent = helper.instrumentMockedAgent()
  , app = require('express')()
  , server = require('http').createServer(app)
- 
+
 
  this.tearDown(function cb_tearDown() {
  server.close()
@@ -177,7 +177,7 @@ test("agent instrumentation of Express 4", function (t) {
  , app = require('express')()
  , server = require('http').createServer(app)
  , api = new API(agent)
- 
+
 
  agent.config.application_id = '12345'
  agent.config.browser_monitoring.browser_key = '12345'
@@ -218,7 +218,7 @@ test("agent instrumentation of Express 4", function (t) {
 
  var app = require('express')()
  , server = require('http').createServer(app)
- 
+
 
  this.tearDown(function cb_tearDown() {
  server.close()
@@ -273,7 +273,7 @@ test("agent instrumentation of Express 4", function (t) {
  var agent = helper.instrumentMockedAgent()
  , app = require('express')()
  , server = require('http').createServer(app)
- 
+
 
  this.tearDown(function cb_tearDown() {
  server.close()
@@ -321,7 +321,7 @@ test("agent instrumentation of Express 4", function (t) {
  var agent = helper.instrumentMockedAgent()
  , app = require('express')()
  , server = require('http').createServer(app)
- 
+
 
  this.tearDown(function cb_tearDown() {
  server.close()