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

Change session behavior #475

Closed
wants to merge 1 commit into from
Closed

Change session behavior #475

wants to merge 1 commit into from

Conversation

baryshev
Copy link

  1. I add support undefined state for maxAge cookie param and set it by default. undefined value for this param means that session will live while browser session live and die together too. Before this time it was impossible (0 or undefined values for maxAge created zero-time session). Also 4, 5, 10 hours for default session live time - wrong way i think, because it application dependent param. Browser session live time looks better for default value.
  2. Before this time session cookie cookies sent with each response. This behavior is prevented caching pages with frontend web-servers like nginx because we must always accept new cookies. Now the cookies sent only once - after creation of session and not prolongate his lifetime until session ends.

Sorry for my bad english, but i hope that you will understand me. Thanks.

@tj
Copy link
Member

tj commented Feb 18, 2012

you could do what you want with req.session.cook.expires = false iirc, but it's pretty obscure (and undocumented). So I'm fine with that change, and I'm fine with it being the default I agree that's a reasonable default, the old default was pretty arbitrary.

IMO we do need "rolling" sessions though, I intend on improving what we have now so that it's not a set-cookie per-request, however it would be terrible UX for a session to expire in 4 hours, then have an active user's session just expire mid-use.

@tj
Copy link
Member

tj commented Feb 18, 2012

ah sorry it's req.session.cookie.expires = null;

@tj
Copy link
Member

tj commented Feb 18, 2012

changed to use that default: f25d85f

@tj tj closed this Feb 18, 2012
@baryshev
Copy link
Author

I agree that it was bad if session breaks until user stop using site. But session is not designed for long life time. If the browser session ended in most cases we are dealing with a new user session. So, I mean that usually not necessary to make the lifetime of the user's ("connect") session to be longer than the lifetime of browser session.

@tj
Copy link
Member

tj commented Feb 18, 2012

yeah i know what you mean, it's not uncommon for people to be setting the session to at least be a few days etc

@baryshev
Copy link
Author

What about sending session cookies with each request? Response which contains cookies can not be cached because it changes the current state. It turns out that it is impossible to cache all responses. Now I looked how it works in PHP. They use the way I suggested above. If the lifetime of the session is set then it ends even if the user is active.

@baryshev
Copy link
Author

Maybe not send cookies in each response, if the default (null) value of maxAge has not been changed. In this case, it will not break the session if it`s lifetime is not equal to the lifetime of the browser session. We do not need to update the cookies that have no parameter "expires". I think it is a good compromise.

@tj
Copy link
Member

tj commented Feb 19, 2012

I just tell varnish to strip the cookie for assets to cache, agreed thought that's definitely a good thing to avoid for browser-session cookies

@baryshev
Copy link
Author

Thanks for fast fix.

@tj
Copy link
Member

tj commented Feb 19, 2012

np. I'm not happy with how the rolling sessions are done I just haven't had much time to fix those

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.

2 participants