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

Separate modification and saving of session data #36

Closed
alexedwards opened this issue Jan 28, 2018 · 2 comments
Closed

Separate modification and saving of session data #36

alexedwards opened this issue Jan 28, 2018 · 2 comments
Labels

Comments

@alexedwards
Copy link
Owner

alexedwards commented Jan 28, 2018

See discussion in #23

This could be addressed by implementing Middleware which uses a custom ResponseWriter to intercept HTTP Responses, save the session data to the store and set the session cookie - similar to v0.1 of SCS (https://github.com/alexedwards/scs/blob/v0.1.0/session/manager.go)

Downside is that this causes problems with frameworks/applications which also try to call WriteHeader. See #15

A workaround might be to set the session cookie header each time the session is modified (or even loaded) and only handle saving to the store in the middleware. I think that would work for most stores, except the CookieStore, which needs the response headers to not have already been written.

@jpfluger
Copy link

jpfluger commented Jan 28, 2018

Ultimately scs is one key ingredient to a greater solution. Since the greater solution is often based on a framework (with its own custom http handlers), it seems that fixing this issue is best left to middleware which would need to check if there's session data awaiting save and then to actually save the session data.

I now have a working example of separating writeHeader from saveStore. Although I touched multiple files, here's a gist for sessions.go. It's backwards compatible - no breaking changes.

I implemented the AutoSave idea successfully tested with Redis but should work for CookieStore. It will need to be cleaned up for locking mutex. In the gist, key modifications include:

  • func write is reworked and now has two additional params for writeHeader and saveStore
  • func put has checks for autoSave and saveCounter
  • func Save should be called at the end of middleware
  • func TouchHeader is called at the beginning of middleware, just after Load. I did look at changing load per @alexedwards suggestion but thought it would be a breaking change to include in manager.Load the ResponseWriter. For this reason TouchHeader was created.
  • all other functions that call s.write are automatically set to write the header and save the data.

Here's a small example of using it in echo middleware

session := config.SessionManager.Load(c.Request())

if config.SessionManager.Opts().IdleTimeout() <= 0 {
	err := session.TouchHeader(c.Response())
	if err != nil {
		c.Error(fmt.Errorf("could not touch header for session; %v", err))
		return nil
	}
} else {
	err := session.Touch(c.Response())
	if err != nil {
		c.Error(fmt.Errorf("could not touch session; %v", err))
		return nil
	}
}

defer session.Save(c.Response())

if err := next(c); err != nil {
	c.Error(err)
}

return nil

Hopefully this helps move the conversation forward.

@alexedwards
Copy link
Owner Author

This issue was fixed as part of the v2.0.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants