Option to save session before sending response

[skoranga]

Looks like fix for #61 (901c8e0) is causing some wired race condition.
In my usecase req.session.save is calling also encrypting the data before storing. And usually it takes more little longer than non-encrypted version. Because of the async nature of session save the next request is starting most of the time before the save is complete. And this next request is fetching the old version of session.
Is there any possibility to add some flag to control the async/sync session save or any other solution?

[dougwilson]

Can you verify if you still have the problem with version 1.7.5? A change was made to mostly address that referenced commit for the case you describe.

[skoranga]

i fear i still can see the issue, but commenting out this code

session/index.js

Lines 216 to 229 in 5c503e7

	if (!isNaN(contentLength) && contentLength > 0) {
	// measure chunk
	chunk = !Buffer.isBuffer(chunk)
	? new Buffer(chunk, encoding)
	: chunk;
	encoding = undefined;
	if (chunk.length !== 0) {
	debug('split response');
	ret = _write.call(res, chunk.slice(0, chunk.length - 1));
	chunk = chunk.slice(chunk.length - 1, chunk.length);
	return ret;
	}
	}

it works fine.

[dougwilson]

Because of the async nature of session save the next request is starting most of the time before the save is complete. And this next request is fetching the old version of session.

can you explain what the "next request" is, exactly?

[skoranga]

first page is one email input and next button. The next page needs carry over of this email and more input fields. So next request is part of multipage form submission.

[dougwilson]

ok. so by "next request is starting most of the time before the save is complete" you mean that you're able to click on the next button on that page before the session has been saved to the store, is that correct? if that is the case, the solution is a feature i have been meaning to add: an option to not send any response at all until the session has been saved to the store. this will make sure that you cannot click the button until the session is in the store.

[skoranga]

yeah exactly the issue.
Thanks looking forward to this feature.

[dougwilson]

just in case you're not aware and feel like you have to wait on me, there is a work-around, which is to call req.session.save yourself and respond within that callback. for example

app.post('/', function (req, res, next) {
  // do stuff
  req.session.email = email
  req.session.save(function (err) {
    if (err) return next(err)
    res.render('page')
  })
})

[skoranga]

Excuse for late reply, but this is not really solving the issue. However I see session save happening twice.

[dougwilson]

However I see session save happening twice.

Oh, you have to set resave: false in your options if you want to use req.session.save, forgot about that, sorry!

[skoranga]

As per https://github.com/expressjs/session/blob/master/index.js#L297, if resave: false, if will check isModified, and it will still try to save.

[dougwilson]

right, but req.session.save will turn isModified to false, thus the second save won't happen if you set resave: false.

[skoranga]

I am afraid it's not working, what I am doing is wrapping res.send in express app in one middleware for all the pages and calling req.session.save inside it with resave: false

[dougwilson]

After you asked about it, I tried it and it worked just fine. I can't really help you with the work-around if I have no idea what your code looks like :) Also, it may be useful to run under DEBUG=express-session to see what is going on.

[skoranga]

I tried DEBUG=express-session as well, i am seeing next fetch log before saved log. Let me work on creating a sample app to reproduce this issue.

[skoranga]

@dougwilson Sorry for late reply again. here is the sample express app which highlights the issue.

If you run this app and access http://localhost:3000/ and click on post & redirect button, it's not saving the session data properly.

To highlight the issue, I have memory-store with get taking 50ms and set taking 80ms. This delay can be because of network latency or some other heavy async operation in store.

[dougwilson]

@skoranga the example does not look like it is implementing the work-around I gave you at #74 (comment) , so of course it's not going to work. I already know it does not work :) But the issue is not easy to fix. You have to use that work-around I gave you above.

[skoranga]

sorry forgot to add the override. updated the sample-app.

Now it's saving, but res.session.save is getting called twice as i mentioned in one of the above comment.

if you insert this console.log('--saving in express-session--'); in node_modules at https://github.com/expressjs/session/blob/master/index.js#L267, you will see:

--saving in app--
--saving in express-session--

[dougwilson]

yes. you need to set resave: false in your sample app like i said above in #74 (comment) :)

[dougwilson]

yes. you need to set resave: false in your sample app like i said above in #74 (comment) :)

nevermind, i see you added resave: false with the update.

[dougwilson]

OK, I see what the issue is. It looks like it has to do with the fact that the session is a new session when the page is loaded and it thinks the session hasn't been saved even after it is manually saved.

[dougwilson]

Actually, the double-save would happen in all situations. The double-saving is definitely a bug, which I'll get fixed, now that I see it, thank you :)

[skoranga]

great. :)

[dougwilson]

Hi @skoranga sorry for the delay. Can you npm install express/session to get the current master version and verify that your code is no longer double-saving for me?

[skoranga]

thanks @dougwilson. it's working as expected with the latest code. So I will still be setting resave:false and overriding req.session.save, right?

[dougwilson]

So I will still be setting resave:false and overriding req.session.save, right?

For now, yes. The resave: false thing won't change until 2.0 (which it is basically slated to be flipped from true to fase). The manually calling of req.session.save may change, yes. I wanted to reference this issue in the commit, but I need to re-open it to track the "save session before sending any of the response" request.

[dougwilson]

The resave: false thing won't change until 2.0 (which it is basically slated to be flipped from true to fase).

I take that back: if you call req.session.save it should not save again, no matter the value of resave.

[skoranga]

Great. thanks for clarification.

thanks for the fix. 👍

[dougwilson]

@skoranga sorry for the re-close :) I was just fixing the new code to not resave after you manually saved regardless of the resave option :) A npm install express/session now should get you the code you really want.

[skoranga]

Yes it works fine. :)

[mjquito]

I still have the same issue as @skoranga was having it. The res.session.save() is saving twice. I'm using "express": "4.16.2", "express-session": "1.15.6". I really need help!

I'm saving my sessions on files, I'm using the session-file-store. The problem I'm trying to solve is that when I call "res.redirect('/')", I noticed in the "./express-session/index.js", the res.end() gets ran and inside of that, the following gets ran

...
if (shouldSave(req)) {
        req.session.save(function onsave(err) {
          if (err) {
            defer(next, err);
          }

          writeend();
        });
...

And then the req.session.save runs asynchronously which then the redirection to \ starts happening and the session gets loaded BUT the req.session.save hasn't finished saving the session in the file which then causes to load the old or previous state of the session. I used the suggested workaround (see below) of calling req.session.save before the redirecting

app.post('/', function (req, res, next) {
  // do stuff
  req.session.email = email
  req.session.save(function (err) {
    if (err) return next(err)
    res.render('page')
  })
})

And it works, but, it is calling twice to the req.session.save which is a no no. I really need help.