medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

Circular invocation when not recursive? (Meteor JS) #81

Closed hexsprite closed 7 years ago

hexsprite commented 7 years ago

I'm having intermittent Circular invocation errors.

Here is the traceback:

    Error: Circular invocation
        at /Users/jbb/co/focuster/node_modules/memoizee/lib/configure-map.js:49:11
        at imports/api/calendar/server/webhooks.js:111:22
        at ctor.baseCalendarWebhook (imports/api/calendar/server/webhooks.js:41:22)
        at boundNext (packages/iron_middleware-stack.js:408:31)
        at runWithEnvironment (packages/meteor.js:1180:24)
        at packages/meteor.js:1193:14
        at ctor.urlencodedParser (/Users/jbb/.meteor/packages/iron_router/.1.1.2.zay7po++os+web.browser+web.cordova/npm/node_modules1/body-parser/lib/types/urlencoded.js:91:37)
        at packages/iron_router.js:886:36
        at Meteor.EnvironmentVariable.withValue (packages/meteor.js:1126:17)
        at ctor.hookWithOptions (packages/iron_router.js:885:27)
        at boundNext (packages/iron_middleware-stack.js:408:31)
        at runWithEnvironment (packages/meteor.js:1180:24)
        at packages/meteor.js:1193:14
        at ctor.jsonParser (/Users/jbb/.meteor/packages/iron_router/.1.1.2.zay7po++os+web.browser+web.cordova/npm/node_modules1/body-parser/lib/types/json.js:103:37)
        at packages/iron_router.js:886:36
        at Meteor.EnvironmentVariable.withValue (packages/meteor.js:1126:17)
        at ctor.hookWithOptions (packages/iron_router.js:885:27)
        at boundNext (packages/iron_middleware-stack.js:408:31)
        at runWithEnvironment (packages/meteor.js:1180:24)
        at packages/meteor.js:1193:14
        at ctor.Router.onBeforeAction.where (server/router.js:172:5)
        at packages/iron_router.js:886:36
        at Meteor.EnvironmentVariable.withValue (packages/meteor.js:1126:17)
        at ctor.hookWithOptions (packages/iron_router.js:885:27)
        at boundNext (packages/iron_middleware-stack.js:408:31)
        at runWithEnvironment (packages/meteor.js:1180:24)
        at packages/meteor.js:1193:14
        at MiddlewareStack.dispatch (packages/iron_middleware-stack.js:432:3)

These are the arguments:

    exception while getting value from cache { name: 'getCalendarListByResourceId',
      arguments: [ 'focusterCalendar', 'nIQmpOHWNzPDGOU4fZEbYGPwZ2A' ] }

This is the actual memoized function, note logArgs is just a wrapper to log the arguments that were passed when the exception occured.

const CACHE_ARGS = { maxAge: 5 * SECONDS, primitive: true }

function logArgs (name, fn) {
  return function () {
    try {
      const result = fn(arguments)
      return result
    } catch (err) {
      logger.warn('exception while getting value from cache', {
        name,
        arguments: Array.prototype.slice.call(arguments)
      })
      throw err
    }
  }
}

export let getCalendarListByResourceId = logArgs(
  'getCalendarListByResourceId',
  memoize(
    (queryBase, resourceId) =>
      CalendarList.findOne({
        [`${queryBase}.watch.resourceId`]: resourceId
      }),
    CACHE_ARGS
  )
)
medikoo commented 7 years ago

@hexsprite stack trace you've shown is most likely incomplete. Be sure to put following as first line in your program

Error.stackTraceLimit = Infinity;

Having that, you should be able to see the stack which will expose circulal invocation.

You can also see that this error cannot really happen otherwise, see logic within memoize

  1. Check if we have cached value for given id -> https://github.com/medikoo/memoizee/blob/f1964ca77117a89dd7ddb022f007af502f5cf363/lib/configure-map.js#L36-L41 if we have return with value from cache
  2. When we don't have, call the underlying function: https://github.com/medikoo/memoizee/blob/f1964ca77117a89dd7ddb022f007af502f5cf363/lib/configure-map.js#L42-L43
  3. Sanity confirmation that cached value for given id still doesn't exist (if it is, it means function was called recursively) -> https://github.com/medikoo/memoizee/blob/f1964ca77117a89dd7ddb022f007af502f5cf363/lib/configure-map.js#L48-L49

I'm closing it, as at this point I don't see bug in memoizee. However if still something is uncertain let's continue discussion here.

hexsprite commented 7 years ago

I think this may be particular to Meteor as it runs everything within a node-fibers environment. The current running Fiber may yield at any time due to a DB request, like in this case.

I think I may be able to somehow make this work with the async version of memoizee if I figure out how to wrap it somehow. Or instead of using a Fiber-based DB call I could use a promise based one.

PS. may be worth adding a note for Meteor users about this Circular Invocation issue in this case as it is a pretty large community.

medikoo commented 7 years ago

PS. may be worth adding a note for Meteor users about this Circular Invocation issue in this case as it is a pretty large community.

Please describe case more precisely, then I may add some note about that, but anyway it's barebones JS, and circular invocation is as simple problem as described. I'm not sure whether adding notes about characterstics of complex ecosystem on top would really help. It may give feeling that things are complicated when they aren't (at memoizee level)

hexsprite commented 7 years ago

Sure, I understand. I think Google will point people here hopefully.

Basically Meteor uses Fibers to avoid callback hell (this was before async/await). What that means is that the code looks synchronous but actually a call to Collection.find() method could in fact allow another Fiber to run while it's waiting for that request.

In my case I was memoizing a DB lookup. So it's pretty likely that while inside the memoize that Fiber paused while it was looking something up, that allowed another memoized DB lookup to run and that caused the error.

Since I switched to using the promise-based Mongo lookup it works fine.

const CACHE_ARGS = { maxAge: 5 * SECONDS, promise: true }
export let getCalendarByCalendarId = memoize(
  calendarId => Calendars.rawCollection().findOne({ _id: calendarId }),
  CACHE_ARGS
)