Solve issue with redirects

[AsafGartner]

When changing the session and then calling res.redirect(...)
express-session calls res.write, which flushes the headers and causes
the browser to start redirecting without waiting for the store to finish
saving.

express-session should not send the headers until the store finishes
saving.

I patched it for now with a hack. It probably requires a more robust
solution.

[dougwilson]

Hi! Your change makes a lot of tests fail, it seems. We cannot accept this until there are no tests failing and perhaps you add a new test that actually tests what is trying to be fixed here.

[dougwilson]

Another possibility if you are not sure what the correct solution is would be to provide some code that uses this module were you are seeing the error. Currently since this module does it's work in res.end, it needs to return a proper true/false response to the call for back-pressure purposes, which is why it writes out (because the user called res.end).

Your "fix" also doesn't work for users who do res.write() then res.end() because then when res.end is called, the headers are already written and there's nothing this module can do about it, so your fix would still not work.

Really, I think you are just not using this module incorrectly--by the time the session is saved, it's too late to do anything to the request. Please show us what you are trying to accomplish, because you are likely just going about it the wrong way.

[BlueHotDog]

Hi,
When doing redirect,
It seems, that since you're doing res.write, it causes the headers to flush, and as the browser picks on the headers it starts doing the redirect.

I'll try to fix the PR but i'm not sure how to add tests for that.

[dougwilson]

I understand what you're saying now--that the user's browser ends up making the request to the HTTP server before the session actually gets saved to the store.

For now, you can work-around this by calling req.session.save and then sending the redirect response in the callback. The real fix is more complicated than your change here and will have to change a lot. For example, even if you change the res.end patch, you'll still be subject to their behavior if you call res.writeHead() or even just res.write() after the redirect header--they will both flush the headers before the session is saved.

I will only accept a PR that accounts for all of those things, otherwise it's just a half-baked change that will just caused even more confusion.

TL;DR, your change needs to work for all these things:

res.redirect('/somewhere')

res.writeHead(302, {Location: '/somewhere'})

res.statusCode = 302
res.setHeader('Location', '/somewhere')
res.write('Redirecting')
res.end()

and I'm sure I've missed some, but your change will ultimately fail for the second two cases.

[BlueHotDog]

right, ok, we'll implement the workaround to our system now, and will work on a better PR later on.

[austencollins]

FYI – The req.session.save workaround does not work. Only 1 out of 4 times will it work in a production app. Is there another workaround in the meantime?

[dougwilson]

The req.session.save workaround does not work. Only 1 out of 4 times will it work in a production app. Is there another workaround in the meantime?

Not that I know of. Can you figure out why it doesn't work and let us know? It seems to work fine with the MemoryStore that ship with this module. Perhaps there is something else going on you can figure out?

[dougwilson]

i'm not sure how to add tests for that.

Be sure there are tests, of course. If you cannot make a test for it, then you're not making a change that makes sense. You have code that fails, so you've basically got a test already; just distill it down to something to go in our test file.

[austencollins]

I'm sorry, the req.session.save may work after all. My specific problem was using MemoryStore in production. I've since switched to 'cookie-session' and had no problems with the redirect issue.

[Fishrock123]

@ac360 keep in mind that cookie-session does not use this module, but rather it's own implementation.

(Edit: sorry, trying out a mobile github client and it weirded out)

[lerouxb]

I just lost hours of my life to this bug ;)

Happens on most requests that modify the session in my vagrant VM, probably because the I/O is relatively slow with the CPU speed largely unaffected. So my browser wins out against the database most of the time.

[Fishrock123]

@lerouxb does @AsafGartner's fix work for you?

Edit: wait, are you using memory store?

[dougwilson]

@lerouxb we still need a real solution here. The current commits just cause massive breakage as I outlined above (though I see they were wrapped in a conditional just so the tests didn't actually test them so it doesn't show the tests as broken--sneaky).

We don't have anyone really maintaining this module, unfortunately.

[dougwilson]

Here is a highlight of how sneaky the commit TheGiftsProject@096f68e was: https://coveralls.io/files/253973625#L251 As you can see, the tests were "fixed" by just avoiding to actually test the change.

To be clear, we do need a fix, but the current change in this PR just isn't going to work :( I wish it was as easy as adding a single line, but doing so will cause issues with stream.pipe(res) at the very least here.

[lerouxb]

@lerouxb I ended up just adding a res.saveAndRedirect() method using custom middleware in the meantime so I can easily find/replace it at a later stage. Using it with my own SessionStore that saves to postgres.

[dougwilson]

I believe I know the true solution to this problem. The issue comes from how this library tries to keep the response from ending until the session is saved, but not delay the rendering of the page. I think the page really should just be delayed for the best (though it could be a switch to change the behavior) but at least there is a way to keep the current behavior and fix this.

The issue comes from setting a Content-Length header and then calling res.end() with the complete length to write, so the delayed res.end() is ignored by browsers, since they just consider the response done as soon as they receive all the bytes in Content-Length.

[dougwilson]

By the way, everyone who is having this issue, I assume you are not using compression() middleware or express.compress(), correct?

[dougwilson]

From looking at @ac360 code, it looks like the answer is no, which confirms my thoughts.

[dougwilson]

Everyone, this should be resolved in version 1.7.4.

[DenysVuika]

This issue is still active with 1.8.2 (express 4.9.5). I've spent several days figuring out why my custom session stores suddenly stopped working after upgrade before coming across this thread. If using any in-memory storage (sync access) redirection works fine, however once I enable any async calls within the store (hitting any database, multiple different providers) - session starts malfunctioning. I observe way too much store.set and store.get calls and very often valid session object gets rewritten with an 'empty' one so destroying authentication (i.e. when using Passport.js).

I tried hundreds of approaches and only explicitly waiting for session save works for me so far:

router.post('/login',
  passport.authenticate('local'),
  function (req, res) {
    req.session.save(function (err) {
      res.redirect('/');
    });
  }
);

Any thoughts?

[dougwilson]

@DenisVuyka yes, for certain types of redirects, you still do have to use that work-around. The only solution is to have this module completely buffer your response and wait for the session to save before then sending it out. This is a planned feature, but it can cause memory issues, of course.

[DenysVuika]

@dougwilson I've just found that connect-flash no longer works as well. req.flash() also gets destroyed during redirects. Does that mean I need to verify every single middleware now to be working correctly and wrap it with session.save calls? Is there any known working version of connect-session I could roll back to?

[dougwilson]

I have never used connect-flash or connect-session, so I have no idea what versions work with what. Also, it would help if you could post a test case to demonstrate the issue (perhaps modifying this module's test suite with a breaking test to demonstrate the issue with connect-flash).

This module has only gotten better than what I used to be. If you want an older version of this module, I assume since you say it used to work, you would know what version you used to be using when it was working?

[dougwilson]

@DenisVuyka I'm going to lock this issue for now so we can discuss this in a new issue without all the unrelated noise above your report. You can open a new issue at https://github.com/expressjs/session/issues/new Please include code necessary for me to reproduce the issue as well as instructions for how I can go about doing that.