Memory leak with sessions.(*CookieStore).New

[arch]

Using Apache benchmark (ab -n100000 -c100) with a simple server that only calls session.Get(r, "session-name") eats up memory very fast. Here's top10 from pprof:

(pprof) top10

Total: 629.5 MB

   406.5  64.6%  64.6%    406.5  64.6% bufio.NewReaderSize

    42.5   6.8%  71.3%     42.5   6.8% net.newFD

    30.0   4.8%  76.1%     30.0   4.8% github.com/gorilla/sessions.(*CookieStore).New

    29.5   4.7%  80.8%     29.5   4.7% github.com/gorilla/context.Set

    29.0   4.6%  85.4%     29.0   4.6% net/textproto.(*Reader).ReadMIMEHeader

    20.5   3.3%  88.6%     69.0  11.0% net/http.ReadRequest

    15.5   2.5%  91.1%     45.0   7.1% github.com/gorilla/sessions.GetRegistry

    12.0   1.9%  93.0%     12.0   1.9% net/http.readTransfer

    11.5   1.8%  94.8%     11.5   1.8% syscall.anyToSockaddr

     7.0   1.1%  95.9%      7.0   1.1% net.sockaddrToTCP

[alex]

Maybe it's not a memory leak, just that GC hasn't collected any garbage yet?

--
 
 

[Kamil Kisiel]

I'd be willing to investigate this some. Can you post your code, details of the benchmarks, how you profiled, etc?

[Dave Cheney]

I do not think this is a leak. I suspect the server side cookie jar expires cookies based on idle time, not size of the jar. As AB will be creating a new session every time in quick succession, I can easily see why the size of the cookie jar would grow alarmingly. 

--
 
 

[Kamil Kisiel]

The server doesn't store anything in the case of the cookie session store. The session data is stored in an encrypted client-side cookie.

[Dave Cheney]

orly. Well, serves me right for not reading the code first.

> --
>
>

[arch]

It's just a trivial server that just calls Get and writes hello:

package main

import (

"fmt"

"github.com/gorilla/securecookie"

"github.com/gorilla/sessions"

"log"

"net/http"

)

var (

store = sessions.NewCookieStore(securecookie.GenerateRandomKey(32))

)

func handler(w http.ResponseWriter, r *http.Request) {

_, _ = store.Get(r, "session-name")

fmt.Fprintln(w, "Hello")

}

func main() {

store = sessions.NewCookieStore(securecookie.GenerateRandomKey(32))

http.HandleFunc("/", handler)

log.Fatal(http.ListenAndServe(":8080", nil))

}

and just run

ab -n100000 -c100 "http://localhost:8080/"

[Kamil Kisiel]

I tried your example with both tip and 1.0.3 of Go and benchmarking with siege with the same settings as your ab. There was no difference in the process's resident set size at any point during the benchmark.

[arch]

?

Tried both tip and 1.0.3 as well here (amd64, linux); memory usage quickly progresses toward 700MB (according to top).

Added _ "net/http/pprof" and run

go tool pprof http://localhost:8080/debug/pprof/heap

after ab -n100000 -c100 "http://localhost:8080/". Here's the output

Adjusting heap profiles for 1-in-524288 sampling rate

Welcome to pprof!  For help, type 'help'.

(pprof) top

Total: 472.5 MB

   320.0  67.7%  67.7%    320.0  67.7% bufio.NewReaderSize

    29.0   6.1%  73.9%     29.0   6.1% net/textproto.(*Reader).ReadMIMEHeader

    24.0   5.1%  78.9%     24.0   5.1% net.newFD

    17.5   3.7%  82.6%     17.5   3.7% github.com/gorilla/context.Set

    15.0   3.2%  85.8%     15.0   3.2% github.com/gorilla/sessions.(*CookieStore).New

    15.0   3.2%  89.0%     32.5   6.9% github.com/gorilla/sessions.GetRegistry

    12.5   2.6%  91.6%     60.5  12.8% net/http.ReadRequest

    12.5   2.6%  94.3%     12.5   2.6% syscall.anyToSockaddr

    10.5   2.2%  96.5%     10.5   2.2% net/url.parse

     8.0   1.7%  98.2%      8.0   1.7% net/http.readTransfer

(pprof)

[arch]

An additional note: commenting out the "store.Get(r, "session-name")" line and running ab again, top(1) reports that the program uses only 5MBs of memory.

[Kamil Kisiel]

Alright, it was my mistake. I was looking for "go run" in the ps output whereas I should have been looking for "a.out". I can confirm your findings. Now to try and figure out the cause...

[Vladimir Mihailenco]

I guess you need to wrap Router with this: https://github.com/gorilla/context/blob/master/context.go#L96

--
 
 

[Kamil Kisiel]

Yep, I just arrived the same conclusion by tracing through the code. You need either ClearHandler to wrap the handlers or use Purge periodically to clear out the contexts. Doesn't seem like this is mentioned in the docs anywhere so it's a bit surprising.

[Shivakumar GN]

On Fri, Nov 16, 2012 at 12:03 AM, Kamil Kisiel <kamil....@gmail.com> wrote:

Yep, I just arrived the same conclusion by tracing through the code. You need either ClearHandler to wrap the handlers or use Purge periodically to clear out the contexts. Doesn't seem like this is mentioned in the docs anywhere so it's a bit surprising.

I just started with gorilla and happened on this thread. The overview section of context package has this to say:

Variables must be cleared at the end of a request, to remove all values that were stored. This can be done in an http.Handler, after a request was served. Just call Clear() passing the request:

context.Clear(r)

...or use ClearHandler(), which conveniently wraps an http.Handler to clear variables at the end of a request lifetime.

The Router from the package gorilla/mux calls Clear(), so if you are using it you don't need to clear the context manually.

This may need better visibility since the relevance may not be obvious when using the sessions package.

[Rodrigo Moraes]

On Nov 16, 10:10 am, Shivakumar GN wrote:
> This may need better visibility since the relevance may not be obvious when
> using the sessions package.

Indeed, sessions docs also need to mention this. Any package that uses
gorilla/context must handle cleanup. It is not very obvious because if
you are not using the default mux handler you need to do it yourself.
Sorry for the confusion and wasted time.

This leakage is explained in the Purge() docs: "This is only used for
sanity check: in case context cleaning was not properly set some
request data can be kept forever, consuming an increasing amount of
memory."

http://www.gorillatoolkit.org/pkg/context#Purge

I actually never experienced it, that was just theoretical. Hehe.

If someone can file an issue, this won't be forgotten. :)

-- rodrigo

[Shivakumar GN]

On Fri, Nov 16, 2012 at 6:15 PM, Rodrigo Moraes <rodrigo...@gmail.com> wrote:

If someone can file an issue, this won't be forgotten. :)

Created one on code.google.com where tickets still to be living.

http://code.google.com/p/gorilla/issues/detail?id=57

[Java Pro]

Hi,

Has the leak issue been fixed in the gorilla/sessions package? 

I searched the sessions package source and did not find the code calling context.clear(). Does this mean you would have to explicitly import the context package, in additional to the sessions package, and call context.clear()? 

How is the leak prevented if only the sessions package is imported? Can someone provide a sample code?

I am assuming you would not have to import the context package after having imported the sessions package. 

Thanks in advance,

JPro

[Kamil Kisiel]

There haven't been any changes in that regard. It seems the issue that was filed wasn't migrated to the GitHub backtracker so this was lost.

I filed https://github.com/gorilla/sessions/issues/15 just now to improve the docs. If you want to have a stab at writing it up please feel free to do so and submit a PR.

[Java Pro]

I just read the source of the "mux" package. It does clear the context data automatically. 

Can I safely conclude that if I am using "mux" and "session" packages together and let the "mux" clear the context data of a request, I don't have to worry about what the session does to the context data and whether the data gets cleared or not by the session since the context will be cleared by the "mux".

Therefore, the context data only needs to be cleared once at the end of the request no matter who clears it. We also don't want to clear the context twice, the first of which is done prematurely before the end of a request. 

If this is the case, we may add some notes to the packages using "context" and state that one and only one "actor" has do the cleaning job. 

Thanks,

JPro

[Java Pro]

Can someone confirm this? If I do not hear back from anyone, I would assume this is true and do not need to investigate further. 

JPro

[Kamil Kisiel]

Yes if you use it in conjunction with the mux package, you should be fine. The issue above was only when mux was not being used.

[Java Pro]

Thanks!