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

continuation-local-storage without domains? #57

Closed
Jeff-Lewis opened this issue Jun 8, 2016 · 11 comments
Closed

continuation-local-storage without domains? #57

Jeff-Lewis opened this issue Jun 8, 2016 · 11 comments

Comments

@Jeff-Lewis
Copy link
Contributor

(branched topic and response to #46 (comment))

@rvagg Thanks for chiming in and providing some feedback. I guess I'm really at a loss and seriously need help from those who have done this and what they recommend. I've been programming for almost 20 years in dozens of languages, frameworks, databases and architectures and I'm still not on the same page as many Node foundation members when it comes to the power and importance of having a reliable asynchronous context.

I love Node because it's lightweight but also can be a heavy hitter when needed (scaling). JS on the server and client streamlines data transfers and provides a high level of architectural flexibility and adaptability.

After using Node for 3+ years now, I still need to learn a 'best practice' architectures, design patterns or whatever you want to call it when it comes to passing context.

As a more concrete example, in our case, here's what we need to do:

Multi-tenant Database (our case MongoDB)

  • ALL queries and writes need to ensure they have a shard key and/or client key and not leave it up to the developer to 'remember' to add it each time they want to query data.
  • ALL changes to data need to be audit-able in order to track who changed what and when.
  • Certain db data areas will need levels of authorization which again should not be left to the developer to remember the right authorization logic every time they want data from these areas. For example, this could be done by appending extra filter clauses to queries.
  • Prefer Promises over callbacks

Ideally, all of this would be done at the data access layer so developer's can focus on building and adapting to business cases, testing and satisfying user stories.

I don't think these, what I consider enterprise-ish, requirements above are asking a lot and I would expect to be able to build an application in Node that could satisfy them.

At first glance, StrongLoop or Mongoose with middleware plugins appear to be solutions, but they are only as reliable as monkey patched shims can be in providing context over asynchronous boundaries.

How does Express or Hapi help with these requirements?
How have others used Node to satisfy these requirements?
Is there something fundamentally wrong with those requirements?

Any thoughts or guidance would be greatly appreciated.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jun 8, 2016

To be clear, I don't recall that Domain and Zones have ever been discussed in this working group. I can't find any evidence of them being discussed in issues or the meeting minutes (except in the context async_wrap and its implementation details). I can understand that you feel Domain and Zones have been neglected, but the discussion should be from the perspective that they where never a part of this working group to start with.


What you are describing can be done without any clever features, though some frameworks may not favor those programming patterns. But I think we should stay on topic which appears to be continuation-local-storage.

continuation-local-storage shouldn't be very hard to implement once async_wrap is finished (and don't worry that continues to be an important part of our and my agenda). Here is a very basic example:

async-hook is just a tiny abstraction on async-wrap that hacks around the current issues we have with timers, nextTick and Promises. If you don't need that, just replace require('async-hook') with process.binding('async_wrap')

'use strict';

const asyncHook = require('async-hook');

function CLS() {
  const ref = new Map();
  const self = this;

  this.storage = null;

  asyncHook.addHooks({
    init(uid, handle, provider, parentUid, parentHandle) {
      ref.set(uid, self.storage);
    },
    pre(uid, handle) {
      self.storage = ref.get(uid);
    },
    post(uid, handle) {
      self.storage = null;
    },
    destroy(uid) {
      ref.delete(uid);
    }
  });
  asyncHook.enable();
}

CLS.prototype.domain = function (cb) {
  const self = this;

  process.nextTick(function () {
    self.storage = {};
    cb();
    self.storage = null;
  });
}

And here is an example with a fake (for simplicity) requestHandler that setup a session and shows a page.

const cls = new CLS();

function errorHandler() {
  setTimeout(function () {
    console.error(`user with id ${cls.storage.userId} caused an error`);
  }, 10 * Math.random());
}

function showPage() {
  setTimeout(function () {
    console.log(`hello ${cls.storage.userName} (id = ${cls.storage.userId})`);
  }, 10 * Math.random());
}

function requestHandler(userId) {
  cls.domain(function () {
    cls.storage.userId = userId;

    setupSession(function (err) {
      if (err) return errorHandler();

      showPage();
    });
  });
}

function setupSession(cb) {
  setTimeout(function () {
    cls.storage.userName = ['Teodor', 'Suljo'][cls.storage.userId];
    cb();
  }, 50 * Math.random());
}

requestHandler(0);
requestHandler(1);

You can expand on this yourself, and I hereby declare it public domain so you may do with it as you wish.

@Jeff-Lewis
Copy link
Contributor Author

@AndreasMadsen this is awesome and thank you for pursuing this. CLS is really all I care about. I refer to Domains as it is (was) the closest officially supported API to provide some kind of context.

Before I build some test modules with what you've provided, is the Asyncwrap Issues List up to date? Does async-hook check any more of those boxes, notably promises? 🙏

@AndreasMadsen
Copy link
Member

Yes the list is up to date. async-hooks adds full nextTick and timers support, some Promises support and does not solve the native addon problem.

The Promises support will work for most intents and purposes, see AndreasMadsen/async-hook#2 for how the patch works.

@Fishrock123 Fishrock123 changed the title Without Domains, how is execution context done using Node? continuation-local-storage without domains? Jun 13, 2016
@Jeff-Lewis
Copy link
Contributor Author

Thanks @AndreasMadsen for the tips and async-hook. FWIW, I made of version of CLS using AsyncWrap (and async-hook) instead of async-listener called cls-hooked.

Knowing it's very beta, I would love to get your thoughts on it.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 23, 2016

Gave it a quick look. It looks correct. I'm not sure where you got currentUid = parentUid || uid;, I can't imagine that would ever be useful. The currentUid = uid you have instead is correct.

Can we close this issue?

@AndreasMadsen
Copy link
Member

Also I'm not sure why you would need:

if (DEBUG_CLS_HOOKED) {
  var stackChain = require('stack-chain');
  for (var modifier in stackChain.filter._modifiers) {
    stackChain.filter.deattach(modifier);
  }
}

is there a bug in one of my modules? :) Anyway, that is a bit off topic.

@Fishrock123
Copy link
Contributor

Closing, if we need to we cab re-open but CLS is definitely something we're doing our best allow good implementations of.

@Jeff-Lewis
Copy link
Contributor Author

Thanks @AndreasMadsen. Yes, if it really should be parentUid = parentUid || currentUid; as you suggested. The stack-chain mods was just a test to see async-hook included in the call stack.

@Fishrock123 Yes, we can close this for now. I just saw #7565 - it's nice to see the discussions and the momentum remains for AsyncWrap.

How do most Node Foundation members see AsyncWrap's purpose - mainly for Tracing and CLS as a pleasant re-purpose for it? I would hope enough would like it to fulfil both Tracing and CLS.

The three fundamental use cases of CLS, Tracing and Error Handling often bring developers together around Domains and other features that overlap and seemingly cause conflicting opinions ( Zones, CLS+Promises ) about how Node should support these use cases.

image

IMHO, I don't think we need one component in the middle that satisfies all. Also, if AsyncWrap were to satisfy Tracing and CLS, it moves us one step closer to being able to remove Domains, especially for those who use Promises as a main strategy for Error Handling across async boundaries.

@Qard
Copy link
Member

Qard commented Jul 25, 2016

For tracing purpose, async_wrap is really just valuable for context propagation. It doesn't remove our need to patch things to collect contextual data, only our need to patch things to propagate context, which is just what CLS is supposed to do.

Error handling is a whole other thing, and opinions on if it should be attached to async_wrap vary. In my opinion, it is a logical place to connect that issue. But it's a complicated issue, so it's easy for it to become problematic, like domains did.

@Jeff-Lewis
Copy link
Contributor Author

It doesn't remove our need to patch things to collect contextual data, only our need to patch things to propagate context, which is just what CLS is supposed to do.

Interesting thought. I'm not sure I'm following unless you're describing how CLS handles context and sits on top of AsyncWrap.

Regarding patching, it it my hope that with more checkmarks in #29, we would eliminate the need to patch any node and user-land modules.

@Qard
Copy link
Member

Qard commented Jul 25, 2016

async_wrap connects the branches in the call tree, but does not provide any information about what the calls in that tree are. We still need to patch the functions we are interested in to get context about what is actually happening. The need for async_wrap in the tracing domain is purely to ensure accurate context tracking.

Patching will not go away, because everyone has different needs for what data should be collected from instrumentation. What would hopefully go away is simply the risk of context loss due to unknown async barriers not getting linked. This unfortunately does not actually get fully solved by async_wrap due to the user-mode queueing problem, but it's a big step to greatly improving the performance and accuracy of that context propagation.

IMO, zones is a better solution, but it would have a somewhat larger performance impact, so something like that would likely need to be implemented in userland, on top of something like async_wrap.

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

4 participants