Saving needed only when session change #672

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@thomascoding

Saving needed only when session changes, otherwise session do save on any page load (why if its not changed?).
I think setTimeout can be replaced. 

Sorry for my bad english ;)

@thomascoding thomascoding Update lib/middleware/session.js
Saving needed only when session changes, otherwise session do save on any page load (why if its not changed?). 
I think setTimeout can be replaced. 

Sorry for my bad english ;)
e2142b7
@langpavel langpavel commented on the diff Oct 14, 2012
lib/middleware/session.js
@@ -276,12 +275,19 @@ function session(options){
res.end = function(data, encoding){
res.end = end;
if (!req.session) return res.end(data, encoding);
- debug('saving');
- req.session.resetMaxAge();
- req.session.save(function(){
- debug('saved');
- res.end(data, encoding);
- });
+ if (originalHash == hash(req.session)) {
+ debug('unmodified session, dont save');
+ setTimeout(function(){
@langpavel
langpavel Oct 14, 2012 Contributor

process.nextTick ?

@tj
tj Jan 13, 2013 Member

I dont get the intention here, we shouldn't need any nextTicks or timeouts in here

@evadnoob
evadnoob Jan 15, 2013

What I noticed happening was "reads" of the session where resulting in
"saves" of the session data. This caused problems when multiple async
requests coming from the same browser process, same session, when one
request modified the session, the other requests with slightly more stale
sessions could potentially overwrite "newer session data".

I think the setTimeout() here is not needed, the important part is the
check for "changed hash"....

-Dave

On Sun, Jan 13, 2013 at 1:54 PM, TJ Holowaychuk notifications@github.comwrote:

encoding

@langpavel langpavel commented on the diff Oct 14, 2012
lib/middleware/session.js
@@ -276,12 +275,19 @@ function session(options){
res.end = function(data, encoding){
res.end = end;
if (!req.session) return res.end(data, encoding);
- debug('saving');
- req.session.resetMaxAge();
- req.session.save(function(){
- debug('saved');
- res.end(data, encoding);
- });
+ if (originalHash == hash(req.session)) {
+ debug('unmodified session, dont save');
+ setTimeout(function(){
+ res.end(data, encoding);
+ },0);
+ }else{
+ debug('saving');
+ req.session.resetMaxAge();
@langpavel
langpavel Oct 14, 2012 Contributor

resetMaxAge called only when session change?
What happens if I login user and don't touch session ever after? Session expires? Very unpractical. This needs be solved first.

@heycalmdown

+1

@adamansky

+1

@evadnoob

+1 I've patched my local connect with a similar change and this has fixed an issue where multiple asynchronous http requests from the same user overwrite session data with stale data(originating from a req.session "get").

@jonathanong
Contributor

never do setTimeout(fn, 0) in node. keep styles consistent - i see a }else{. also, address langpavel's comments.

needs a test, rebase, and squash. ping me when you've made these changes!

@thomascoding

jonathanong i cant make this changes on github. Also i cant test because i am currently don't use this lib. As you see other people make changes and this code solve problem.

@jonathanong
Contributor

Okay I'll just close this for now. Thanks

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