sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

Compression option is broken, it doesn't unload data #13

Closed fabn closed 9 years ago

fabn commented 9 years ago

When compressed data size is below the compression threshold data loading is completely wrong. It will return the compressed data.

This is likely to happen when cached data has low entropy and compress well (html, json, etc). You should really put a big warning on this because in production this can make disasters.

Attached is a failing spec, the main issue is in this line, you cannot check if value is compressed comparing its compressing size with threshold value.

To fix that in a reliable way you should save a compression flag along with compressed data, or you can try to guess if compression is on by looking at first bytes and check if they matches zlib format.

However I think that the very best option is to introduce some metadata in data you store (e.g. first byte contain your flags), in this way in the future you can swap compressors and support other options, dalli does that in this way.

Since this it's a pretty big change (and potentially a breaking change) I won't try to submit a fix before discussing this with you.

I'd like to switch to redis for caching to get rid of memcache extensions but this is for me a stopper issue.

sorentwo commented 9 years ago

@fabn Thank you very much for not only reporting this, but also providing a failing test case. I took your test case and implemented the simplest backward compatible change possible in 535f724. Would you mind taking a look and testing it out?

fabn commented 9 years ago

I checked that and it seems fine as soon as you control deflate params. If any param is changed (e.g. compression level set to Zlib::BEST_SPEED) or made user configurable that won't work anymore.

However as a quick fix for current version that's absolutely fine, thanks for your prompt response to this issue.

I'll open another issue for a feature request related to this.