Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Dec 8, 2018

Conversation

secracon
Copy link
Contributor

@secracon secracon commented Dec 5, 2018

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
Copy link
Contributor Author

secracon commented Dec 5, 2018

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

sessions.go Show resolved Hide resolved
Copy link
Contributor

@elithrar elithrar left a comment

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
Copy link
Contributor

elithrar commented Dec 8, 2018

Thanks @secracon!

@secracon
Copy link
Contributor Author

secracon commented Dec 8, 2018

You're welcome!

Copy link

@giganteous giganteous left a comment

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
Copy link

aaslamin commented Apr 17, 2019

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

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.

None yet

6 participants