rfink / sequelize-redis-cache

Small fluent interface for caching sequelize database query results in redis more easily
MIT License
175 stars 47 forks source link

Same cache used for query with different where #31

Open Naoto-Ida opened 7 years ago

Naoto-Ida commented 7 years ago

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.

FranciZ commented 7 years ago

@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 commented 7 years ago

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

rfink commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

rfink commented 7 years ago

We have a proposed fix at https://github.com/rfink/sequelize-redis-cache/pull/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 commented 6 years ago

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 commented 5 years ago

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.