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
Don't memoize results that had an error #1466
Conversation
// #1465 don't memoize if an error occurred | ||
if (!err) { | ||
memo[key] = args; | ||
} | ||
var q = queues[key]; | ||
delete queues[key]; | ||
for (var i = 0, l = q.length; i < l; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep this as is for now, but for v3, I'd be open to changing this behaviour, as there's a potential race condition here (similar to #1248).
If the initial call to the memoized function takes between m
and n
seconds to err, and a second call is made x
seconds after the initial (m < x < n
) then the function might only be run once depending on whether or not the initial call finished. To eliminate it, we would have to call the memoized function again if there are queued calls, and an error occurred.
Yes, this is a potential 3.0 change. It's debate-able as to whether caching errors is "correct" or not, though. e.g. caching the result of a request that 404's -- do you cache the 404, or keep retrying? With this change, you could use |
This is ready to be merged for 3.0. Also, this new behavior is much less surprising if you were using an |
Includes a fix for memoize memoize no longer memoizes errors (caolan/async#1465, caolan/async#1466)
fixes #1465
This one is debatable if we want to change behaviour in 2.x. I think it makes sense to however could be swayed otherwise. Thoughts @aearly?