Skip to content

- use golang context pkg instead of gorilla/context to fix memory leaks - #175

Merged
elithrar merged 3 commits into
gorilla:masterfrom
secracon:master
Dec 8, 2018
Merged

elithrar merged 3 commits into
gorilla:masterfrom
secracon:master

Conversation

@secracon

@secracon secracon commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

If gorilla/sessions is used together with other solutions including gorilla/mux, then there is a big chance of memory leaks, because the request may change during it's lifetime due to shallow copying of the request when the request context is changing. Once the request is copied, and that copy is used instead, then the request can no longer be found as key in the global context store managed by gorilla/context. This way the expired contexts are remaining in memory forever.

@secracon

secracon commented Dec 5, 2018

Copy link
Copy Markdown
Contributor Author

This change won't work with golang versions below 1.7 as the context pkg did not exist for those versions

Comment thread sessions.go

@elithrar elithrar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of last requests, @secracon -

Let me know if you need me to pick this up if you're unable to find time.

@elithrar
elithrar merged commit 12bd476 into gorilla:master Dec 8, 2018
@elithrar

elithrar commented Dec 8, 2018

Copy link
Copy Markdown
Contributor

Thanks @secracon!

@secracon

secracon commented Dec 8, 2018

Copy link
Copy Markdown
Contributor Author

You're welcome!

@giganteous giganteous left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The go.mod file still references github.com/gorilla/context. Did you run go mod tidy?

@aaslamin

aaslamin commented Apr 17, 2019 •

Copy link
Copy Markdown

Hi @elithrar, will there be a tagged release with this change?

The latest tag v1.1.3 does not include this commit: v1.1.3...master

Reason being is that as a consumer I want to avoid locking down to a commit and rely on semver instead so my dep manager does the heavy lifting.

@rojakcoder

Copy link
Copy Markdown

I think @elithrar will release a minor version when he can find time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants