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

Rework cache provisioning apis #3876

SimonSchick opened this issue Nov 12, 2018 · 4 comments

Rework cache provisioning apis #3876

SimonSchick opened this issue Nov 12, 2018 · 4 comments
breaking changes Change that can breaking existing code


Copy link

There is a few issues with how caches are defined at the moment:

When specifying caches in new Server(...) all values are deep cloned (individual cache settings are also cloned individually as well), this causes issues with any caching strategy that allows passing it data structures that have reference based state (sockets etc.).
See hapijs/catbox-redis#80 (comment)

This is a general problem with hoek.clone itself.
I know there is a discussion on that, we should look into stopping it cloning class instances.

My team, myself and my friends using hapi entirely stopped using options.cache entirely due the issue mentioned above and are only using server.cache.provision with engine being a direct engine instance eg. s.cache.privision({ cache: new CatboxMemory(...) })

Demo time, all of the console.log statements below should print true:

void((async () => {
    void(h = require('hapi'));
    void(m = require('catbox-memory'));
    void(a = new m());

    // Scenario 1, config based provision
    void(s = new h.Server({ cache: [{ engine: a, name: 'test' }] }));
    console.log(s._core.caches.get('test').client.connection === a); // fails due to `m` being deep-cloned

    // Scenario 2, active provision
    void(await s.cache.provision({ engine: a, name: 'test2' }));
    console.log(s._core.caches.get('test2').client.connection === a); // this works

    // Scenario 3, active provision but indirect constructor
    void(r = require('ioredis'));
    void(b = new r())
    void(cr = require('./dist/backend/app/server/caches/catbox-redis'));
    void(await s.cache.provision({ engine: cr, name: 'redis', client: b }));
    console.log(s._core.caches.get('redis').client.connection.client === b); // this doesn't work due to cache options cloning



The server cache options seem too flexible, often leading to errors in correct usage as this allows 4 ways to achieve the same thing.
In combination with (1) can lead to entirely unexpected errors when trying to pass through engine constructor options

I propose that we restrict the configuration to the following:

new Server({
  cache: [{ // Can still be a single object.
    engine: new CatboxMemory({
      partition: 'anything', // optional
    }), // required
    name: 'myOwnCache', // required, maybe use symbol for default cache instead of omitting key to make it more obvious
    shared: true // optional

The same restriction should apply to server.cache.provision as well, this should be trivial since both, configuration and provision use the same internals.

This would be a breaking in hapi and catbox with very little update time involved.

This shouldn't require any changes to existing catbox engine implementations.

Copy link

I agree that the current API is problematic. I have my own workaround, where I make a custom engine in options.cache:

cache: [{
    name: 'redis',
    partition: 'cache',
    engine: function (options) {

        options.client = new IoRedis();
        return new CatboxRedis(options);

Copy link

@kanongil so you are using engine as a factory function rather than a constructor? Guess that's a way go about it 😛

Copy link

Yup. It's not pretty but it works.

@hueniverse hueniverse transferred this issue from outmoded/discuss Nov 12, 2018
@hueniverse hueniverse self-assigned this Jan 6, 2019
@hueniverse hueniverse added the breaking changes Change that can breaking existing code label Jan 6, 2019
@hueniverse hueniverse changed the title Proposal: Re-work the way caches are provisioned Rework caches provisioning Jan 6, 2019
@hueniverse hueniverse added this to the 18.0.0 milestone Jan 6, 2019
@hueniverse hueniverse changed the title Rework caches provisioning Rework cache provisioning apis Jan 7, 2019
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
breaking changes Change that can breaking existing code
None yet

No branches or pull requests

3 participants