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

Same cache used for query with different where #31

Open
Naoto-Ida opened this issue Mar 2, 2017 · 9 comments
Open

Same cache used for query with different where #31

Naoto-Ida opened this issue Mar 2, 2017 · 9 comments

Comments

@Naoto-Ida
Copy link
Contributor

Naoto-Ida commented Mar 2, 2017

I have a setup like this:

var cacher = require('sequelize-redis-cache');
var redis = require('redis');
var Sequelize = require('sequelize');

var rc = redis.createClient(6379, 'localhost');
var db = new Sequelize('cache_tester', 'root', 'root', { dialect: 'mysql' });
var cacheObj = cacher(db, rc)
  .model('sequelize-model-name')
  .ttl(5);
const find1 = await cacheObj.find({attributes:['name'], where: { type: 'user', confirmed: true } })
const find2 = await cacheObj.find({attributes:['name'], where: { type: 'admin', confirmed: true } })

When I log the queries,
SELECT 'name' FROM 'sequelize-model-name' WHERE id = 2 is run twice...
At first I thought the use of async/await seemed to cause the issue,
but changing to then didn't change the fact.

@Naoto-Ida Naoto-Ida changed the title Unintended cache used Same cache used for query with different where Mar 2, 2017
@FranciZ
Copy link

FranciZ commented Aug 6, 2017

@Naoto-Ida @rfink This is a major issue with this library and I can't figure out exactly what's causing it. If using with PM2 cluster mode it's even more ridiculous. It returns results from other queries all the time which makes this very dangerous to use in production. Just write your own caching. There are some good functions to use from this library but don't use it as it is. It's broken.

It took me a couple of days to realise that it's this library that's the cause of the issue and it's really dangerous. Have rewritten the caching using some of the ideas from this library and it now works as expected.

@rfink
Copy link
Owner

rfink commented Aug 6, 2017

@FranciZ would you mind sharing what you have? I'd like to track this issue down, seems very serious.

@rfink
Copy link
Owner

rfink commented Aug 6, 2017

@Naoto-Ida can you send me the full code, as well as what version you are using of:

node
mysql
mysql (node lib)
sequelize-redis-cache (node lib)
redis (node lib)

To be able to reproduce? I haven't been able to reproduce myself yet.

@FranciZ
Copy link

FranciZ commented Aug 6, 2017

@rfink

I can't share the exact code without revealing too much about the project. Best I can do is describe the case. I have a Node express app that receives a lot of http traffic and used this library to cache the SQL data for faster response times. I didn't experience any problems during development but after deploying with PM2 cluster and throwing a lot of traffic at it the methods started returning results from different queries. It's not common but at peak concurrent traffic every once and a while a wrong instance gets returned. For example:

UserModel.findOne({ where:{id:1}})
  .then(user => console.log(user));

would actually return an instance of the ProjectModel... I went through the library trying to identify the source of the issue with no success. I ended up implementing custom caching using pretty much the same approach but without the prototype and module and it works as expected.

I initially suspected a Redis issue but I've excluded that as a possibility having it work with the custom implementation.

I'm using Node 7.2.0 for this project, sequelize 3.30.2, sequelize-redis-cache 2.0.0, ioredis 2.5.0, redis 3.2.6, postgres 9.6.

If I understand @Naoto-Ida correctly he was able to reproduce this problem by simply making one query immediately after the other and receiving the same result from both queries.

@rfink
Copy link
Owner

rfink commented Aug 7, 2017

Thanks for the info. I tried the example from @Naoto-Ida above, and was not able to reproduce. I'm going to keep digging into this issue, please let me know if you do encounter a reproducible case. Apologies for what's happening, this is very strange.

@FranciZ
Copy link

FranciZ commented Aug 7, 2017

@rfink Thanks for looking into it. I'll try to create a reproducible case I can share.

shaozujie added a commit to shaozujie/sequelize-redis-cache that referenced this issue Aug 17, 2017
rfink added a commit that referenced this issue Aug 17, 2017
@rfink
Copy link
Owner

rfink commented Aug 17, 2017

We have a proposed fix at #34, which is now released as 2.0.1. I'm going to keep this issue open, @FranciZ, to see if you can/want to confirm that this helps.

@JacobT14
Copy link

JacobT14 commented Jun 25, 2018

I don't think this is completely fixed. I've created a GraphQL App using this and it seems whenever I pass a where statement it always uses the cache regardless of the value passed into the parameter...


import { GraphQLID, GraphQLString, GraphQLNonNull } from 'graphql'
import db from '../../../models/index.js'
var cacher = require('sequelize-redis-cache')
var cacheObj = cacher(db.sequelize, db.rc)
  .model('endgame')
  .ttl(60)
import models from '../../../models/index.js'
import { EndGame } from '../../types/endgame.js'

export default {
  type: EndGame,
  args: {
    game_id: {
      type: new GraphQLNonNull(GraphQLID)
    }
  },
  resolve(root, args) {
    conolse.log(args)
    return cacheObj.findOne({
      where: {
        game_id: args.game_id
      }
    })
  }
}

@aravindvaideesh
Copy link

We did face a similar issue in our production server. When there were a lot of concurrent connections at peak load redis did misbehave. Ours is a ticketing application . We faced lot of complaints from our end users that they had booked for a particular movie but the booking had got registered for another movie. We narrowed the issue down to sequelize-redis after a lot of analysis. However we are unable to reproduce this in our local systems or staging servers. We are planning to replace this package and write a wrapper ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants