Jeff-Lewis / cls-hooked

cls-hooked : CLS using AsynWrap or async_hooks instead of async-listener for node 4.7+
BSD 2-Clause "Simplified" License
758 stars 89 forks source link

When should runPromise exit context ? #71

Open Nickxingyu opened 2 years ago

Nickxingyu commented 2 years ago

I think runPromise should exit context before return promise ! Otherwise, the context can be modify by operations outside this context.

Namespace.prototype.runPromise = function runPromise(fn) {
  let context = this.createContext();
  this.enter(context);

  let promise = fn(context);
  if (!promise || !promise.then || !promise.catch) {
    throw new Error('fn must return a promise.');
  }

  if (DEBUG_CLS_HOOKED) {
    debug2('CONTEXT-runPromise BEFORE: (' + this.name + ') currentUid:' + currentUid + ' len:' + this._set.length + ' ' + util.inspect(context));
  }

  this.exit(context)
  return promise
    .then(result => {
      if (DEBUG_CLS_HOOKED) {
        debug2('CONTEXT-runPromise AFTER then: (' + this.name + ') currentUid:' + currentUid + ' len:' + this._set.length + ' ' + util.inspect(context));
      }
      return result;
    })
    .catch(err => {
      err[ERROR_SYMBOL] = context;
      if (DEBUG_CLS_HOOKED) {
        debug2('CONTEXT-runPromise AFTER catch: (' + this.name + ') currentUid:' + currentUid + ' len:' + this._set.length + ' ' + util.inspect(context));
      }
      throw err;
    });
};
AJKarnitis commented 1 year ago

I mentioned this in #73, but if we exit where you have proposed, then the context gets ripped out from underneath the runPromise call if it is actually async and needs to get/set any values from its context after it awaits its calls.

This IS a problem though; I think it's a much deeper issue though since promises open the door to there needing to be multiple "current" contexts and I'm not seeing a good way around that.