Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When should runPromise exit context ? #71

Open
Nickxingyu opened this issue Jul 18, 2022 · 1 comment
Open

When should runPromise exit context ? #71

Nickxingyu opened this issue Jul 18, 2022 · 1 comment

Comments

@Nickxingyu
Copy link

Nickxingyu commented Jul 18, 2022

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants