mcollina / async-cache-dedupe

Async cache with dedupe support
MIT License
640 stars 40 forks source link

Invalid string length at StorageRedis.clearReferences #89

Closed 11laflame closed 1 year ago

11laflame commented 1 year ago

[11:22:07.302] ERROR (28): acd/storage/redis.clearReferences error err: { "type": "RangeError", "message": "Invalid string length", "stack": RangeError: Invalid string length at Object.write (/usr/app/node_modules/ioredis/built/Pipeline.js:310:29) at EventEmitter.sendCommand (/usr/app/node_modules/ioredis/built/Redis.js:387:28) at execPipeline (/usr/app/node_modules/ioredis/built/Pipeline.js:330:25) at Pipeline.exec (/usr/app/node_modules/ioredis/built/Pipeline.js:282:5) at StorageRedis.clearReferences (/usr/app/node_modules/async-cache-dedupe/src/storage/redis.js:323:56) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async StorageRedis._invalidateReferences (/usr/app/node_modules/async-cache-dedupe/src/storage/redis.js:227:5) at async StorageRedis.invalidate (/usr/app/node_modules/async-cache-dedupe/src/storage/redis.js:193:16) at async Cache.invalidateAll (/usr/app/node_modules/async-cache-dedupe/src/cache.js:185:5) at async Promise.all (index 0) }

When I try to invalidate array of references, catching this error Using chunks, maximum array of 10, if we had a lot, but it's even crushes with 4 I think maybe because I'm operating with big data in redis, but library should control this

mcollina commented 1 year ago

Thanks for reporting!

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

11laflame commented 1 year ago

@mcollina Hi

https://github.com/11laflame/cache-bug-setup

check please, follow readme steps

mcollina commented 1 year ago

The problem in your example is that you are setting the model directly in https://github.com/11laflame/cache-bug-setup/blob/e630b47da14594664ad7de4ea1b88da097cbfe51/src/services/fill.js#L11, without using the methods from async-cache-dedupe to set the cache. This creates a bad data structure in redis, making the server crash happen. I've tried hard to reproduce just using this module (no fastify, no Mercurius, no mercurius-cache) but I failed, could you try?

11laflame commented 1 year ago

@mcollina It's imitating reference functionality in mercurius-cache, it's required to use this flow

policy: { test: { references: generateReferences, key: generateCacheKey }, }

generateReferences returning array ["model:sdj3-3dsd3-sddsd", "model:2dkd3-33dsdd2-2d2"]

mcollina commented 1 year ago

I've tried hard to reproduce just using this module (no fastify, no Mercurius, no mercurius-cache) but I failed, could you try?

simone-sanfratello commented 1 year ago

I've reproduced the issue with the example you provided, thank you for providing that.

Looking at the code in the example, looks like you want to use the invalidation as it was a feature detached from this module, is that correct?

https://github.com/11laflame/cache-bug-setup/blob/main/src/services/fill.js#L11

https://github.com/11laflame/cache-bug-setup/blob/main/src/services/invalidate.js#L10

11laflame commented 1 year ago

Yes, you are right, it's imitating "references" functionality, and invalidation I'm actually using like here. If you think I'm imitating wrong, you can do it in your way, but problem will still persist

simone-sanfratello commented 1 year ago

Ok, thank you for clarifying.

Unfortunately, invalidation can't work in that way, that functionality is designed to keep references and keys synced and it can't work out of that context.

Probably you can achieve what you want to do using something like this (note this code doesn't work as it is)

const keys = await redicClient.keys(`model:*`)
const removes = keys.map(key => ['del', key])
await redicClient.pipeline(removes).exec()

Anyway, I'm going to fix the case you described, thank you.

11laflame commented 1 year ago

So right now I can not invalidate through unique ids? And I should call all records in this model using this example

mbkkong commented 1 year ago

+1

simone-sanfratello commented 1 year ago

@11laflame if you use the async-cache-dedupe methods, you can set entries and invalidate them, for example https://github.com/mcollina/async-cache-dedupe/blob/main/test/storage-redis.test.js#L367

If you write straight into redis, bypassing the async-cache-dedupe methods, invalidation can't work, and you have to delete entries straight from redis

11laflame commented 1 year ago

@simone-sanfratello Sorry for disturbing you so often but the last commit with fix didn't get to the latest release 2.0.0

commit: e2d0bd54855a5b1c1b8733e0cfbc26421746a6fc

image

simone-sanfratello commented 1 year ago

@11laflame the invalidation feature can't work as you need to use it, its operation is tailored to this library, so the fix is intended to avoid errors at runtime

11laflame commented 1 year ago

@simone-sanfratello but it's working while redis have small size of data inside even with references and key

policy: { model: { references: generateReferences, key: generateCacheKey }, } generateReferences returning array ["model:12", "model:15"]

and app.graphql.cache.invalidate(['model:12', 'model:15']) crushes only when it have a lot of data to check

simone-sanfratello commented 1 year ago

That's not what was in the https://github.com/11laflame/cache-bug-setup

I'm happy to debug it, please setup a piece of code to reproduce that scenario, thanks