Repository navigation
Conversation
Use request's standard context (context.Context) instead of Gorillar's Context. You DON'T need to wrap your handlers with Gorillar context.ClearHandler(). There is no memory leak.
There was a problem hiding this comment.
I appreciate the PR, but one of the reasons we've struggled with making this change sooner is that it's extremely API breaking: there's an extensive discussion in #80 surrounding this. I've deliberated over this for a long while and it's been pretty frustrating.
Critically, the major challenges are because Go doesn't have a great versioning story. golang/dep is great, but not widely used yet, and gorilla/sessions has a lot of existing users and dependent libraries that don't pin versions.
In order of priorities:
-
Not breaking the Store interface is of utmost importance. It's far reaching, and requires multiple authors working together to get that right. Some Stores aren't actively maintained, and work today. We can submit PRs to many stores to get them working, but those stores then ALSO need to version. I think we can achieve this by still keeping the
Savesignature the same, and just ignoring the*http.Requestunder the new context. -
Not breaking the rest of the API, which is mostly the Registry (which is what handles the context stuff the most). This is harder, because you need to add
*http.Requestas an argument, which makes this harder: you can't 'cheat' that like you can the Store interface. -
Not islanding Go 1.6 users, who can't use
request.WithContext- I raised an issue over ongorilla/muxthat describes some of the pain here.
Ideally, I had hoped that golang/dep would land in Go 1.10, which would be the catalyst for adopting versioning here. The steps after that are still non-trivial:
- Getting our most critical importers using
depand pinning to the old version of mux (a lot of work!) - Making the breaking change with a major SemVer bump - "2.0.0" (easy, but this is where the pain comes in)
- Dealing with the fallout of the long tail of users who inevitably run into issues - good docs on how to address, triaging issues, etc. (ongoing)
I hope that makes sense.
| // | ||
| // It returns a new session if there are no sessions registered for the name. | ||
| func (s *Registry) Get(store Store, name string) (session *Session, err error) { | ||
| func (registry *Registry) Get(r *http.Request, store Store, name string) (session *Session, err error) { |
There was a problem hiding this comment.
This changes the public API [breaking change]
| // the response or returning from the handler. | ||
| func (s *Session) Save(r *http.Request, w http.ResponseWriter) error { | ||
| return s.store.Save(r, w, s) | ||
| func (s *Session) Save(w http.ResponseWriter) error { |
There was a problem hiding this comment.
This changes the public API [breaking change]
|
|
||
| // Save should persist session to the underlying store implementation. | ||
| Save(r *http.Request, w http.ResponseWriter, s *Session) error | ||
| Save(w http.ResponseWriter, s *Session) error |
There was a problem hiding this comment.
This changes the public API [breaking change]. It would break all existing store implementations, of which there are many.
| // Save adds a single session to the response. | ||
| func (s *CookieStore) Save(r *http.Request, w http.ResponseWriter, | ||
| session *Session) error { | ||
| func (s *CookieStore) Save(w http.ResponseWriter, session *Session) error { |
There was a problem hiding this comment.
This changes the public API [breaking change]
|
As a note, I'd like to address this "soon", by:
|
elithrar
changed the title
|
Is this waiting on Go modules to be enabled out of box in 1.13 or 1.14? |
|
This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days. |
3 participants
Use request's standard context (context.Context) instead of Gorillar's Context. You DON'T need to wrap your handlers with Gorillar context.ClearHandler(). There is no memory leak.