gin-contrib / cache

Gin middleware/handler to enable Cache
https://gin-gonic.github.io/gin
MIT License
365 stars 95 forks source link

Cache in InMemoryStore is broken #61

Open fzlee opened 4 years ago

fzlee commented 4 years ago

You may find the source code at https://github.com/fzlee/GoldenFly. it's a personal blog

In all, the following code demonstrates the way I use this middleware

// main.go,  init memory store
store := persistence.NewInMemoryStore(5 * time.Second)
// page/router.go, creating cached view
router.GET("/articles-sidebar/", cache.CachePage(store, time.Second * 10, PageSideBarView))

You may reproduce this issue by the following steps:

  1. goto http://ifconfiger.com
  2. open web developer tool
  3. refresh this web page for several times

there are chances we could get an invalid JSON response from the server like this: Screen Shot 2019-12-24 at 13 15 33

seems the memory block which we used to keep response cache is modified by other requests.

axengine commented 4 years ago

gin可能使用了内存池或者sync.Pool等技术来管理内存,可能对[]byte的复用,而InMemoryStore直接缓存了gin的[]byte,因此可能被覆盖。重写cache的Write方法可以解决该问题。

func (w *cachedWriter) Write(data []byte) (int, error) {
    ret, err := w.ResponseWriter.Write(data)
    if err == nil {
        store := w.store
        var cache responseCache
        if err := store.Get(w.key, &cache); err == nil {
            cache.Data = append(cache.Data, data...)
        } else {
            cache.Data = make([]byte, len(data))
            copy(cache.Data, data)
        }

        //cache responses with a status code < 300
        if w.Status() < 300 {
            val := responseCache{
                w.Status(),
                w.Header(),
                cache.Data,
            }
            err = store.Set(w.key, val, w.expire)
            if err != nil {
                // need logger
            }
        }
    }
    return ret, err
}
zxmrlc commented 4 years ago

@axengine 你好,这个方法已经在类中更新,但是仍然存在问题. image 我现在的库已经包含了你发布的这个函数. 但是还是存在缓存返回错误. 缓存本来应该返回json格式的内容,却混入了日志等信息...

axengine commented 4 years ago

@zxmrlc 对缓存数据要先分配内存,做一次强拷贝,你的代码还是直接缓存了上层传过来的[]byte。 使用CachePageAtomic替换CachePage方法,CachePage在并发时会引起缓存数据错误。

zxmrlc commented 4 years ago

@axengine 首先谢谢您的回复.. 我使用的就是CachePageAtomic, 你说的这个重写Write方法.. 是指申请内存然后进行操作吗?直接在cache库更改代码不太好吧..

axengine commented 4 years ago

@zxmrlc 我fork了cache项目,修复了这个问题,你可以直接 https://github.com/axengine/cache

zxmrlc commented 4 years ago

https://github.com/axengine/cache

感谢您的回复

chenjiandongx commented 4 years ago

@axengine @appleboy Hi,

I get a little confused about the effect of the following code. What case will trigger this logic?

func (w *cachedWriter) Write(data []byte) (int, error) {
    ret, err := w.ResponseWriter.Write(data)
    if err == nil {
        store := w.store
        var cache responseCache
                 // start  -------
        if err := store.Get(w.key, &cache); err == nil {
                         // why does it need a append operation?
            data = append(cache.Data, data...)
        }
                 // end  -------
        //cache responses with a status code < 300
        if w.Status() < 300 {
            val := responseCache{
                w.Status(),
                w.Header(),
                data,
            }
            err = store.Set(w.key, val, w.expire)
            if err != nil {
                // need logger
            }
        }
    }
    return ret, err
}
appleboy commented 4 years ago

@axengine Can you send the PR?

axengine commented 4 years ago
                 // start  -------
      if err := store.Get(w.key, &cache); err == nil {
                         // why it needs a append operation?
          data = append(cache.Data, data...)
      }
                 // end  -------

对一个大buff可能需要多次Write才能写完。

chenjiandongx commented 4 years ago

@axengine Write 不是只被调用了一次吗?

https://github.com/gin-contrib/cache/blob/master/cache.go#L177

morefreeze commented 4 years ago

@zxmrlc 我fork了cache项目,修复了这个问题,你可以直接 https://github.com/axengine/cache

尝试了你的项目github.com/axengine/cache v1.2.0,写了一段测试代码类似

store := persistence.NewInMemoryStore(time.Second * 2)
apiR.GET("/xxx", cache.CachePage(store, time.Second*2, GetSth))

然后用for i in {1..20}; do echo $i; curl www.abc.com/xxx > $i & done 并发去打,很容易就复现各文件大小不一致,接口返回一个固定的大字典,大小约200k

htaoji1988 commented 3 years ago

我也遇到了,希望早点解决内存做缓存的问题

seriousm4x commented 1 year ago

ran into the same issue today. i can verify it