hapijs / catbox Goto Github PK

1. Mongo Cache caches non-error responses:
   expected null to exist

Would be nice to have the logic that is used to generate the http cache headers to be exposed as a utility method so users that need to provide explicit cache headers can use the same config values as used in the route definitions.

In version 3.x, calling policy.get() returned a callback with the signature function (err, cached). This change makes the callback signature consistent so that it always use the same signature: function (err, item, cached, report) which was already used when generateFunc option is provided.

While developing the memcache engine, I noticed infrequent, but often enough, errors from the Policy file. They weren't always the same, but here are a few I have seen.

$ npm test

> [email protected] test /Users/jchapel/Projects/catbox
> make test-cov



  ..................................................
  ..........................................x.......
  ..................................................
  ..................................................
  .....................................


 1 of 237 tests failed:

  1) Policy Stale returns the processed cached item using manual ttl:

      actual expected

      12

      at /Users/jchapel/Projects/catbox/test/policy.js:850:51




 No global variable leaks detected.

Coverage: 100.00%
Coverage succeeded
make: *** [test-cov] Error 1
npm ERR! weird error 2
npm ERR! not ok code 0

$ npm test

> [email protected] test /Users/jchapel/Projects/catbox
> make test-cov



  ..................................................
  ............................................x.....
  ..................................................
  ..................................................
  .....................................


 1 of 237 tests failed:

  1) Policy Stale returns stale object then invalidate cache on error when calling a helper using the cache with stale config:
     TypeError: Cannot read property 'gen' of undefined
      at /Users/jchapel/Projects/catbox/test/policy.js:909:42
      at /Users/jchapel/Projects/catbox/lib/policy.js:704:8
      at null._onTimeout (/Users/jchapel/Projects/catbox/test/policy.js:784:28)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)




 No global variable leaks detected.

Coverage: 99.65%

lib/policy.js missing coverage on line(s): 173, 174
Code coverage below threshold: 99.65 < 100
make: *** [test-cov] Error 1
npm ERR! weird error 2
npm ERR! not ok code 0

I am running Node.js version 10.7 currently on OSX 10.8.4.

These are also ran from chapel/catbox#master, I will check to see if the same issues arise from Spumko/catbox#master.

hapijs/catbox-memcached#5 (comment)

To avoid Race Condition we need to provide a way in catbox to handle CAS.

The problem is each Database implements its own.

For MongoDB, you use findAndUpdate so you can basically use the previous document and the next one and be sure that the update will work only if the previous document didn't changed.

For Redis, you can use the WATCH command to create a transaction.

For Memcached we have the cas command that takes a casunique to make sure nobody raced us when storing the new version.

How do you guys see how we can implement the cas command to catbox?

Hey,

I am a happy user of your library, but as my project grows we have a lot of dependencies.
We could save a lot of space if you added .npmignore file to your project.

I even found tool that can help you with it: https://github.com/inikulin/dmn

Thanks!

When using toCache, the return for Policy.getOrGenerate will be either the object containing the toCache method for the case of freshly initialized data or the return of toCache for cached data.

This was a bit confusing as I expected the data sent to the callback to be the same in either situation.

The cache expiration timeout is run in the context of the current domain, meaning a reference is retained to that request in hapi environments and other environments the cache cleanup may be invalidated due to the current domain being disposed.

Under some circumstances it's necessary to http cache 404s and similar and it would be nice to have a method to opt in using the route cache configuration or similar.

Hi there.

We have a use case for a cache that continues to serve stale content if the service that provides fresh values is slow or unavailable. Catbox almost does this for us, returning a stale value on the first request once a value is stale, but drops the stale value from the cache if there is an error in the generate function.

Would you be open to a pull request that makes this configurable through a option in the options passed into Policy, keeping the current behaviour as default?

It's unclear if cache objects returned by the Policy APIs are guaranteed to be modification safe or if consumers of these APIs need to clone or perform other operations in order to prevent modification of the cache contents by downstream data consumers.

I'm pretty sure this is an issue with NPM itself, but just for the awareness: I and some others have been able to reproduce this.

$ npm install catbox
npm ERR! Error: shasum check failed for /var/folders/4r/1dk0h6n507l047j316_vnnw80000gp/T/npm-49505-bQYT2iKp/registry.npmjs.org/catbox/-/catbox-3.3.0.tgz
npm ERR! Expected: a8200f666d62d43873c7aef8ab38518bc3411efd
npm ERR! Actual:   da39a3ee5e6b4b0d3255bfef95601890afd80709
npm ERR! From:     https://registry.npmjs.org/catbox/-/catbox-3.3.0.tgz
npm ERR!     at /usr/local/lib/node_modules/npm/node_modules/sha/index.js:38:8
npm ERR!     at ReadStream.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/sha/index.js:85:7)
npm ERR!     at ReadStream.emit (events.js:117:20)
npm ERR!     at _stream_readable.js:943:16
npm ERR!     at process._tickCallback (node.js:419:13)
npm ERR! If you need help, you may report this *entire* log,
npm ERR! including the npm and node versions, at:
npm ERR!     <http://github.com/npm/npm/issues>

npm ERR! System Darwin 13.3.0
npm ERR! command "node" "/usr/local/bin/npm" "install" "catbox"
npm ERR! cwd /Users/jaaprood/Sites/test
npm ERR! node -v v0.10.31
npm ERR! npm -v 1.4.26

It seems to only affect 3.3, 3.2 is fine.

When using getOrGenerate, there is no way to monitor cache error rates, performance, etc as the set call's handler is eventually pushed to Hoek.ignore.

There should be some way of exposing catbox errors at any point in the system and ideally there would be generic ops-level tracking so things like the delay for set calls, etc can be tracked upstream.

List the common ones first, then the ones specific to each engine type (including host/port).

Since Catbox requires an object containing the methods in the initialization, it is impossible to use the Hapi CLI with a plain json configuration and load a custom storage module.

An idea of how to fix this is if the engine specified is not one of the known backends, check node_modules for a match and attempt to use that module as the backend.

Replaces #93

When a pending timeout returns for a previous request, and a current request for the same id intersect, the timeout returns the old stale item.

Given the easy of object hashes and the absence of concurrent access, it should be pretty easy to implement a memory based backend.
Without peeking into the code, why is it not production ready?

1. Mongo #start connects with authentication:
   expected [Error: Database authentication error: {"name":"MongoError","errmsg":"auth fails","ok":0}] to not exist

I have use case where I use dynamic ttl which is returned from generateFunc. But since ttl can have wide range of values, I can't use staleIn, which is shame, because it makes cache lot nicer.

So I have following suggestion, so please provide feedback if you like it and I would turn it to PR.

staleIn and staleTimeout in Policy options could be number of milliseconds OR function with signature function(createdAt, ttl), where ttl would be remaining ttl. Therefore Policy object would use these functions to dynamically calculate staleIn and staleTimeout for given createdAt and ttl.

Thank you for quick feedback :-).

Ideally the framework should monitor what get requests are still in flight and chain all such calls to reduce upstream load. I.e.

get('foo', callback1)
get('foo', callback2)
(callback1 called)
(callback2 called)

Should only call the relevant engine method once.

Force implementors to manually install (or include in their app package.json) the modules they actually use. Need to ensure the hapi tests continue to work after this change.

For example in https://github.com/spumko/catbox/blob/master/lib/memory.js#L90 instead of:

return callback(null, result);

use:

return process.nextTick(function(){
    callback(null, result);
});

This will ensure your callback is async.

Best,

JB

Thanks for building a really cool package. We're building an app with Hapi and we've decided to 'use strict' everywhere.

When doing this, I came across an issue/bug when strict mode is enabled globally which includes our hapi modules:

node_modules/hapi/node_modules/catbox/lib/policy.js:146

SyntaxError: In strict mode code, functions can only be declared at top level or immediately within another function.

Here is the offending code:

function timerFunc() {

                if (wasCallbackCalled) {
                    return;
                }

                wasCallbackCalled = true;
                return callback(null, cached.item, cached, report);
}

Strict mode complains because ECMA standards specify that function declarations are not valid within blocks and timerFunc is being declared within another functions block.

Let me know if you need any other help/assistance with getting this fixed.

Right now, with staleTimeout, if it takes a ridiculously long time to get data (which will be added to the cache after it's been loaded) on the very first request, the whole process blocks until that data is loaded. Once it's stored in the cache, of course everything is fine.

I'd really love to have a situation where even if there's no data, the cache returns null after a designated timeout, but then continues to load in the background. This way we can keep the user moving with slightly less data and then show the data when it finally arrives.

I have been contemplating adding multi-get/set to catbox, since a few of the clients support it natively (memcached, redis, mongo, and technically memory). Ones that don't support it natively can still have the methods, but wrapped with loops and warnings that you shouldn't use these unless you know the potential performance impacts.

My question for @hueniverse and @geek, would you be okay with PRs on catbox and the clients we support for this functionality?

Javascript is asynchroneous and sometimes we want to get a list of keys.
It could be nice to have the gets method to the catbox client so we could get a list of results.

redis and memcached would have the gets for free but for other cache backends, we could just use Async.map to do it in javascript.