@instana/core - npm Package Compare versions

Comparing version 2.11.0 to 2.11.1

CHANGELOG.md

@@ -6,2 +6,13 @@ # Change Log
 
+## [2.11.1](https://github.com/instana/nodejs/compare/v2.11.0...v2.11.1) (2022-11-09)
+
+
+### Bug Fixes
+
+* **sdk:** work around memory leak with recursive SDK usage ([c9b35eb](https://github.com/instana/nodejs/commit/c9b35eb37f1e41f7b11442dd408ca53f5cb2ac13))
+
+
+
+
+
 # [2.11.0](https://github.com/instana/nodejs/compare/v2.10.0...v2.11.0) (2022-11-04)
@@ -8,0 +19,0 @@

package.json

 {
   "name": "@instana/core",
-  "version": "2.11.0",
+  "version": "2.11.1",
   "description": "Core library for Instana's Node.js packages",
@@ -139,3 +139,3 @@ "main": "src/index.js",
   },
-  "gitHead": "c326cc5b0ce29b5ca89158b128d192750b243600"
+  "gitHead": "3d29a8b21bc9aad1daa4e8512c7763f5cfb844c1"
 }

src/tracing/clsHooked/async_local_storage_context.js

@@ -112,3 +112,4 @@ /*
   /**
-   * Creates a new CLS context in this namespace.
+   * Creates a new CLS context in this namespace as a child of the currently active context (or as a root context if no
+   * context is currently active).
    * @returns {InstanaCLSContext}
@@ -126,2 +127,14 @@ */
   /**
+   * Creates a new root CLS context in this namespace, independent of the currently active context.
+   *
+   * @returns {InstanaCLSContext}
+   */
+  createRootContext() {
+    const context = Object.create(Object.prototype);
+    context._ns_name = this.name;
+    context.id = executionAsyncId();
+    return context;
+  }
+
+  /**
    * Runs a function in a new CLS context. The context is left after the function terminates. Asynchronous work started
@@ -128,0 +141,0 @@ * in this function will happen in that new context. The return value from that function (if any) is discarded.

src/tracing/clsHooked/context.js

@@ -13,2 +13,7 @@ /*
 
+// Changes:
+// - rename the symbols to avoid name clashes
+// - have Namespace.prototype.set return a function which can be used to unset the value from the context
+//   on which it was originally set.
+
 'use strict';
@@ -86,3 +91,4 @@
 /**
- * Creates a new CLS context in this namespace.
+ * Creates a new CLS context in this namespace as a child of the currently active context (or as a root context if no
+ * context is currently active).
  */
@@ -98,2 +104,12 @@ Namespace.prototype.createContext = function createContext() {
 /**
+ * Creates a new root CLS context in this namespace, independent of the currently active context.
+ */
+Namespace.prototype.createRootContext = function createRootContext() {
+  let context = Object.create(Object.prototype);
+  context._ns_name = this.name;
+  context.id = currentUid;
+  return context;
+};
+
+/**
  * Runs a function in a new CLS context. The context is left after the function terminates. Asynchronous work
@@ -100,0 +116,0 @@ * started in this function will happen in that new context. The return value from that function (if any) is discarded.

src/tracing/clsHooked/index.js

@@ -6,27 +6,4 @@ /*
 
-/* eslint-env es6 */
-/* eslint-disable */
+/* eslint-disable max-len */
 
-// This is a copy of cls-hooked
-
-// We are using a variation of cls-hooked, because we need to add additional cleanup logic.
-// cls-hooked in its published version does not permit removal of values added to contexts.
-// This is problematic for us in cases where sockets are long lived, e.g. http agents with
-// maxSockets: Infinity. In such cases, the addition of the Node.js collector and the values it
-// adds to async contexts (which are kept alive due to the living sockets), can tip a Node.js
-// process over the edge.
-//
-// See also:
-// https://github.com/Jeff-Lewis/cls-hooked/issues/21
-// https://github.com/Jeff-Lewis/cls-hooked/issues/11
-
-// Changes:
-// - rename the symbols to avoid name clashes
-// - have Namespace.prototype.set return a function which can be used to unset the value from the context
-//   on which it was originally set.
-
-// Copy of
-// Jeff-Lewis, feat(compat): v4.2 for node v4.7-v8 (0ebfb9b  on Jul 21, 2017)
-// https://github.com/Jeff-Lewis/cls-hooked/blob/066c6c4027a7924b06997cc6b175b1841342abdc/index.js
-
 'use strict';
@@ -33,0 +10,0 @@

src/tracing/clsHooked/unset.js

@@ -34,3 +34,3 @@ /*
   // 1) If there is an async leak in the Node.js runtime, that is, missing destroy calls for async_hook resources for
-  // which an init call has been received, the memory used by clsHooked will grow, because context objects will be  kept
+  // which an init call has been received, the memory used by clsHooked will grow, because context objects will be kept
   // around forever. By keeping the reduced span we can at least see (in a heap dump) for which type of spans the
@@ -43,3 +43,2 @@ // destroy call is missing, aiding in troubleshooting the Node.js runtime bug.
   if (key === currentSpanKey && span != null) {
-    const gqd = span.gqd;
     context[reducedSpanKey] = {
@@ -52,3 +51,5 @@ n: span.n,
     };
+
     // Also keep captured GraphQL destination (host & port) for subscription updates, if present.
+    const gqd = span.gqd;
     if (gqd) {
@@ -55,0 +56,0 @@ context[reducedSpanKey].gqd = gqd;

src/tracing/sdk/sdk.js

@@ -29,3 +29,3 @@ /*
   /** @type {Function} */
-  let wrapper = null;
+  let runInContext = null;
 
@@ -67,2 +67,3 @@ /**
       constants.SDK.ENTRY,
+      true,
       startEntrySpan,
@@ -147,2 +148,3 @@ /** @type {Object.<string, *> | null} */ (tags),
       constants.SDK.INTERMEDIATE,
+      false,
       startIntermediateSpan,
@@ -212,2 +214,3 @@ tags,
       constants.SDK.EXIT,
+      false,
       startExitSpan,
@@ -256,2 +259,3 @@ tags,
    * @param {string} sdkKind
+   * @param {boolean} forceRootContext
    * @param {Function} stackTraceRef
@@ -264,4 +268,15 @@ * @param {Object.<string, *>} tags
    */
-  function startSdkSpan(name, kind, sdkKind, stackTraceRef, tags, traceId, parentSpanId, callback) {
-    return wrapper(() => {
+  function startSdkSpan(name, kind, sdkKind, forceRootContext, stackTraceRef, tags, traceId, parentSpanId, callback) {
+    const context = forceRootContext
+      ? // SDK entry spans: Force ALS/CLS to use a new, separate root context. There is nothing we ever want to fetch
+        // from a parent/ancestor context for SDK entry spans. This avoids a memory leak when client code is calling
+        // sdk.startEntrySpan recursively in the same context over and over again, which creates an evergrowing chain
+        // of contexts, linked by prototypical inheritance.
+        cls.ns.createRootContext()
+      : // Let ALS/CLS create a new child context of the currently active context. This is the correct behavior for
+        // intermediate and exit spans. We need them to happen in a child/descendant context of the currently active
+        // entry span.
+        null;
+
+    return runInContext(() => {
       const span = cls.startSpan('sdk', kind, traceId, parentSpanId);
@@ -280,3 +295,7 @@ span.stack = tracingUtil.getStackTrace(stackTraceRef);
       return callNext(callback);
-    });
+
+      // The additional argument `context` for runInContext (cls.ns.runAndReturn or cls.ns.runPromise, depending on the
+      // API type) allows us to force a specific context to be used by those functions. We use a new root context for
+      // SDK entry spans and null otherwise (intermediate spans, exit spans).
+    }, context);
   }
@@ -348,3 +367,3 @@
     cls = _cls;
-    wrapper = isCallbackApi ? cls.ns.runAndReturn.bind(cls.ns) : cls.ns.runPromise.bind(cls.ns);
+    runInContext = isCallbackApi ? cls.ns.runAndReturn.bind(cls.ns) : cls.ns.runPromise.bind(cls.ns);
   }
@@ -351,0 +370,0 @@

New alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Fixed alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Improved metrics

Total package byte prevSize

656001

increased by0.22%

Lines of code

17765

increased by0.14%

No dependency changes