From 30870932b5035de35f9985fa66ab8ed78a5a9970 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Tue, 20 Jun 2017 23:12:54 -0700 Subject: [PATCH] work around context loss with async-listener MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [async-listener][1] is important module for APM tools and for long stack traces. Promises make the concept of a long-stack-trace ambiguous – as you can conceive the `then` callback as a continuation of either the resolution context or the context that queued the `then` callback ([more details][2]). async-listener defaults to the resolution context. This is the wrong default for how we are using promises here, resuling in APM tools like Stackdriver Trace losing context. We work-around the problem by not using the promise after it has resolved. [1]: https://www.npmjs.com/package/async-listener [2]: https://github.com/othiym23/node-continuation-local-storage/issues/64 --- index.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/index.js b/index.js index dff0c48..dccac86 100644 --- a/index.js +++ b/index.js @@ -33,6 +33,26 @@ class Auth { } getAuthClient (callback) { + if (this.authClient) { + // This code works around an issue with context loss with async-listener. + // Strictly speaking, this should not be necessary as the call to + // authClientPromise.then(..) below would resolve to the same value. + // However, async-listener defaults to resuming the `then` callbacks with + // the context at the point of resolution rather than the context from the + // point where the `then` callback was added. In this case, the promise + // will be resolved on the very first incoming http request, and that + // context will become sticky (will be restored by async-listener) around + // the `then` callbacks for all subsequent requests. + // + // This breaks APM tools like Stackdriver Trace & others and tools like + // long stack traces (they will provide an incorrect stack trace). + // + // NOTE: this doesn't solve the problem generally. Any request concurrent + // to the first call to this function, before the promise resolves, will + // still lose context. We don't have a better solution at the moment :(. + return setImmediate(callback.bind(null, null, this.authClient)); + } + var createAuthClientPromise = (resolve, reject) => { var googleAuth = new GoogleAuth();