Memoize causes queued callbacks to jump across domains

[owenallenaz]

Async.memoize queues up callbacks to prevent a cache-stampede. The result of this is that if you call the same function two times before it's completed, then the second callback will actually be executed in the context of the first. This can cause it to jump across domains because the 2nd callback was originally in the context of another domain, but because it was queued, is now put into the context of the 1st domain. This is solved almost identically to the way that the node mongodb driver handles this same problem. I have added a test case for the scenario.

[aearly]

I'd prefer to not have to deal with domains inside Async, especially since they are deprecated. Could you bind your callbacks manually when you pass them to memoize?

[owenallenaz]

You can, but it's an odd abstraction. Here would be my counter arguments:

1. It forces application developers to know the inner workings of all of their libraries that way they have to bind a callback that goes through any library that may use memoize internally. Basically any time a library changes global state, it's a problem, and dumping a domain because we use async fits into the same realm I believe. Do you know every library that uses memoize? Me neither, but if I use domains in my application, I now need to know which library calls I need to bind.
2. The deprecation is a bit odd honestly, as we see here deprecate domains nodejs/node#66 in the later comments by Trev Norris, domains won't be removed until there is a replacement. I've searched for a bit but honestly at this moment I haven't found any replacement proposal which is gaining any traction. Basically some node core members hate domains because they believe thrown errors should crash process, but everyone is resigned to the fact that there needs to be some way to handle async error management.
3. The patch I've submitted is backward and forward compatible. In the event process.domain doesn't exist, be it browser or maybe even a newer version of node which replaces them, the code will continue to function properly.
4. The change is only like 3 or 4 lines of code. In the event Node does finally replace domains, the likely replacement can't be many more lines of code.
5. This is a common change required in basically every library that does callback queueing. Most commonly this is done in database drivers (mongodb, mysql) because they utilizes a connection pool which creates a callback array. It's the same fundamental problem as asyncs memoize.
6. I think your argument would apply if we were binding each async.series or async.parallel into a new domain and utilizing the domain as a form of error management. This branch simply causes it to pass along the domain if the calling application happened to be using one, in this way we aren't altering global state, were simply maintaining it.

It's not a deal breaker, we can work around it, but I believe it's the right thing to do. If you want to sit on it for a while and see if other people upvote, that a possibility.