fix(context): fixed runPromise mem leak and incorrect propagation

[eliabecardoso]

This PR is fixing:

• Issue #63. Memory leak and incorrect propagation of the runPromise method.
• Issue #71. The exit method was being called after the promise was resolved/rejected, it looks like it was not clearing the active property in time for the next createContext called on the next runPromise.

No side effects occured after this fix.

Tests:

1. Before fix:

2. After fix:

3. Inner Propagation fixed:

[AJKarnitis]

But if you actually have a promise in your runPromise call, wouldn't this cause the context to get ripped out from underneath the inner call?

e.g. if you had:

ns.set('foo', 'outer');
await ns.runPromise(async () => {
  ns.set('foo', 'inner');
  await new Promise((res, rej) => setTimeout(() => res(null), 100));
  const val = ns.get('foo');
  console.log(`expected inner, got ${val}`);
});

wouldn't that log 'outer'?

I'm not sure if we can have our cake and eat it here without a much larger change to how cls is handling the 'current' context and I'm not sure what that would even have to look like.

I stumbled upon this because I was looking to see if there was an issue/PR because I'm definitely running into a similar issue to what you're trying to fix; I'm trying to use this in conjunction with nestJS and Sequelize to get automatic transaction handling, but I'm running into issues where multiple http requests are using the same context and overwriting each others values because I don't think the way this is setup is built to handle async/parallel usage...

The scenario I'm personally struggling with essentially boils down to:

const firstPromise = ns.runPromise(async () => {
  ns.set('foo', 'first');
  await new Promise((res, rej) => setTimeout(() => res(null), 100));
  const val = ns.get('foo');
  console.log(`expected first, got ${val}`);
});
const secondPromise = ns.runPromise(async () => {
  ns.set('foo', 'second');
  await new Promise((res, rej) => setTimeout(() => res(null), 100));
  const val = ns.get('foo');
  console.log(`expected second, got ${val}`);
});
await Promise.all([firstPromise, secondPromise]);

Without your fix, I get: 'expected first, got second' 'expected second, got second'

But with your fix, I get: 'expected first, got undefined' 'expected second, got undefined'

[Jef-Z]

Hi, @eliabecardoso @AJKarnitis

I'm running into issues where multiple http requests are using the same context and overwriting each others values

I finally found the place where this was mentioned. Yes. I had this problem too. It manifests itself as: When we print the log in the promise and get the context in the log:

• Sometimes it got another context (For example it gets the wrong userid)
• or it can't get the context. (The context is undefined)

Have you solved this problem yet?

[eliabecardoso]

Hello @AJKarnitis @Jef-Z. Sorry for the long time without answering.

I also had the same problem, but in my case it was with async lib (v1) in legacy codes. To call async.waterfall, async.paralell or similaries. Contexts overwriting each others in multiples http requests too. I analyzed that the problem was caused by a lack of treament in the async methods, where it lost the bind of who called it. To solve, in the global or startup file I forced the rebind with the cls-hooked, it was this code that solved my problem:

// global.js
// ...
/**
 * Async lib in its v1 has loss of reference and binding issues.
 * Something that doesn't happen in v2 and v3, but we have differences in the way we use the methods that prevent us from upgrading the version.
 * This is a forced rebind was created to palliatively solve the problem.
 */
const waterfall = async.waterfall.bind(async);
const parallel = async.parallel.bind(async);

function bindContextWrap(tasks, callback) {
  const mustBind = process.versions.node < '16.17';
  const storage = ContextStore.getInstance().storage;

  const refTasks = tasks.map(task => (mustBind ? storage.bind(task) : task));
  const refCallback = mustBind ? storage.bind(callback) : callback;

  return this(refTasks, refCallback);
}

async.waterfall = bindContextWrap.bind(waterfall);
async.parallel = bindContextWrap.bind(parallel);

Note:

• ContextStore is a wrapper library that use cls-hooked (minor node 16.17) or async_hooks (node 16.17 or major) to store request contexts.
• storage is the same as cls namespace for cls-hooked, ie namespace.bind(task) / namespace.bind(callback).

My PR is mainly focused on fixing memory leaks and bad propagation in functions that don't lose their context. Therefore, in some cases, you will need to correct in ways similar to the above code. Cls-hooked clearly not prepared to handle async contexts, but this lib is the best we have for your proposal in node versions below 16.17.

I hope this explanation brings a light at the end of the tunnel to the problem you are facing.