Save session before the location header is sent to the browser

[patriksimek]

This PR fixes the problem described in #69. When user calls res.redirect(), it checks whether the session needs to be saved (or touched) before the location header is sent do the browser.

This fix doesn't work for situation when user sets the location header manually.

Test included.

[dougwilson]

Please propose a solution that is not specific to Express.js; it should work when users set the Location header manually; in fact, this is broken because it'll add res.redirect() even when someone is not using Express and if they call it, it'll fail.

[dougwilson]

This throws is the user is not using Express; this library works outside on Express in things like Connect as well.

[patriksimek]

Thanks for the review, now it's not specific to Express and also it should correctly pass all the arguments to proxied method.

I'm not sure if it is possible to fix this for situation when user sets the location header manually. It can't be done in setHeader neither in end method. I'm out of ideas here.

[dougwilson]

and also it should correctly pass all the arguments to proxied method.

Can you add tests for this, then? To me, that does not seem to be the case at all. The following all need to be tested to work here: res.redirect(308, url), res.redirect(url, 308), and res.redirect(url). The tests should also validate that the status code is 308.

[dougwilson]

Thanks for the review, now it's not specific to Express

Well, if it's not specific to Express, how will people without res.redirect not still encounter the issue?

I'm not sure if it is possible to fix this for situation when user sets the location header manually. It can't be done in setHeader neither in end method. I'm out of ideas here.

Right, it's really hard, which is why the issue is still outstanding. If someone can come up with a solution it'll be accepted :) The goal of the modules in the expressjs organization is not to cater only to Express, but rather work in any framework that accepts Express-like middleware. I'm also not too keen on the large code duplication (which needs a lot more tests, as indicated from the dropped code coverage :( ).

[patriksimek]

So, as you mentioned here, the only solution is to buffer the response. I have updated my PR with this kind of patch that should work as neutral middleware. I have also added an option for this feature so it could be published as non-breaking feature.

Code duplication and test coverage should be fine now.

[dougwilson]

The contract with .writeHead is that we must call it sync right away; we cannot actually delay it's call, sadly. This is a limitation with injecting ourselves directly into the http response object without an abstraction.

The reason is that once code calls res.writeHead(), then Node.js guarantees that the side-effects have occurred sync, i.e the following must work, but this PR breaks it:

res.writeHead(500)
assert(res.statusCode === 500)

[patriksimek]

I have updated the code with fix for statusCode, but if the problem is in delaying writeHead, than you can close this right now :)

[dougwilson]

The status code was just an example of the issue. For instance, that may "fix" the status code, but the following will still fail:

res.writeHead(500)
assert(res.headersSent === true)

as well as:

res.writeHead(204)
assert(res.chunkedEncoding === false)

which can induce very subtle bugs in existing applications.

[dougwilson]

Won't this not work if the user calls the following, since you are not looking at the already-set response code?

res.statusCode = 301
res.setHeader('Location', '/')
res.end()

[patriksimek]

It should be fine, implicit writeHead is always called with already-set response code.

https://github.com/nodejs/io.js/blob/3777f415625ce538de5edbb19f7330356da190a8/lib/_http_server.js#L150

This case is also covered by added test should buffer the response #2.

[patriksimek]

I agree this feature is a breaking one so it should not be enabled by default. Users could be notified via docs about fact that enabling this feature may cause some properties to not being set immediately. Just an idea...

[dougwilson]

So this code is assuming that calling res.writeHead will invoke your res.writeHead sync, but we are purposely violating this, so in the spirit of that, we need to assume that someone else replaced res.writeHead that is also not sync (for example, what if they are using two express-session modules?).

[dougwilson]

I'm going to close this since nothing has been changed from the issues pointed out in comments over a year ago.