From d327eb79273dafd76e73f302bc4688f16c900386 Mon Sep 17 00:00:00 2001
From: Stephen Belanger <admin@stephenbelanger.com>
Date: Thu, 15 Mar 2018 13:58:42 -0700
Subject: [PATCH] fix(span): Do not pass stack frames into promises (#269)

Passing error stack frames into a promise context makes it
uncollectable by the garbage collector.
---
 lib/instrumentation/span.js | 55 ++++++++++++++++++++++++-------------
 package.json                |  1 +
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/lib/instrumentation/span.js b/lib/instrumentation/span.js
index f5c278e5a1..e08d2e3c4e 100644
--- a/lib/instrumentation/span.js
+++ b/lib/instrumentation/span.js
@@ -5,6 +5,7 @@ var debug = require('debug')('elastic-apm')
 var Timer = require('./timer')
 var stackman = require('../stackman')
 var parsers = require('../parsers')
+var Value = require('async-value-promise')
 
 module.exports = Span
 
@@ -33,7 +34,9 @@ Span.prototype.start = function (name, type) {
   this.name = name || this.name || 'unnamed'
   this.type = type || this.type || 'custom'
 
-  if (!this._stackObj) this._recordStackTrace()
+  if (this._agent._conf.captureSpanStackTraces && !this._stackObj) {
+    this._recordStackTrace()
+  }
 
   this._timer = new Timer()
 
@@ -102,7 +105,33 @@ Span.prototype._recordStackTrace = function (obj) {
     obj = {}
     Error.captureStackTrace(obj, Span.prototype.start)
   }
-  this._stackObj = obj
+
+  var self = this
+
+  // NOTE: This uses a promise-like thing and not a *real* promise
+  // because passing error stacks into a promise context makes it
+  // uncollectable by the garbage collector.
+  var stack = new Value()
+  this._stackObj = stack
+
+  // TODO: This is expensive! Consider if there's a way to cache some of this
+  stackman.callsites(obj, function (err, callsites) {
+    if (err || !callsites) {
+      debug('could not capture stack trace for span %o', {id: self.transaction.id, name: self.name, type: self.type, err: err && err.message})
+      stack.reject(err)
+      return
+    }
+
+    if (!process.env.ELASTIC_APM_TEST) callsites = callsites.filter(filterCallsite)
+
+    var next = afterAll((err, res) => {
+      err ? stack.reject(err) : stack.resolve(res)
+    })
+
+    callsites.forEach(function (callsite) {
+      parsers.parseCallsite(callsite, false, self._agent._conf, next())
+    })
+  })
 }
 
 Span.prototype._encode = function (cb) {
@@ -111,23 +140,11 @@ Span.prototype._encode = function (cb) {
   if (!this.started) return cb(new Error('cannot encode un-started span'))
   if (!this.ended) return cb(new Error('cannot encode un-ended span'))
 
-  if (this._agent._conf.captureSpanStackTraces) {
-    // TODO: This is expensive! Consider if there's a way to cache some of this
-    stackman.callsites(this._stackObj, function (err, callsites) {
-      if (!callsites) {
-        debug('could not capture stack trace for span %o', {id: self.transaction.id, name: self.name, type: self.type, err: err && err.message})
-        done()
-        return
-      }
-
-      if (!process.env.ELASTIC_APM_TEST) callsites = callsites.filter(filterCallsite)
-
-      var next = afterAll(done)
-
-      callsites.forEach(function (callsite) {
-        parsers.parseCallsite(callsite, false, self._agent._conf, next())
-      })
-    })
+  if (this._agent._conf.captureSpanStackTraces && this._stackObj) {
+    this._stackObj.then(
+      value => done(null, value),
+      error => done(error)
+    )
   } else {
     process.nextTick(done)
   }
diff --git a/package.json b/package.json
index bddc71a98a..992f321f63 100644
--- a/package.json
+++ b/package.json
@@ -65,6 +65,7 @@
   "homepage": "https://github.com/elastic/apm-agent-nodejs",
   "dependencies": {
     "after-all-results": "^2.0.0",
+    "async-value-promise": "^1.0.0",
     "console-log-level": "^1.4.0",
     "cookie": "^0.3.1",
     "core-util-is": "^1.0.2",