Skip to content

Ensure the session is touched before the cookie is set - #296

Closed
sboehler wants to merge 7 commits into
expressjs:masterfrom
sboehler:master
Closed

Ensure the session is touched before the cookie is set#296
sboehler wants to merge 7 commits into
expressjs:masterfrom
sboehler:master

Conversation

@sboehler

@sboehler sboehler commented Mar 29, 2016

Copy link
Copy Markdown

No description provided.

@sboehler sboehler mentioned this pull request Mar 29, 2016
Closed
@gabeio gabeio added the pr label Mar 30, 2016
@sboehler sboehler closed this Mar 30, 2016
@sboehler sboehler reopened this Mar 30, 2016
@sboehler

sboehler commented Mar 30, 2016

Copy link
Copy Markdown
Author

I initially struggled quite hard to find out what's happening in the patched _end method and found the separation of commit activities into things that happen 'on header' and in _end a bit arbitrary. Initially, to fix #276, I had the (simple) suggestion to touch the session twice, one 'on headers' and once in '_end' - admittedly not very elegant.

So I had another go and unified all session-handling code in 'on headers' and '_end' into one clean method. I believe my PR improves the code for a number of reasons:

  • All session-related activities are cleanly isolated in commitSession
  • The _end override is much simpler and readable
  • it does not matter anymore whether the 'onHeaders' listener or 'end' is called first - the execution order is always the same (fixes cookie.expire should be modified to match cookie.maxAge #276)
  • it is a lot more clear whether the session code is async or sync
  • setting the cookie can be related to the other session commit activities, so future work on the code should be simpler and less error prone

All tests pass, including the new one for #276 , however I saw coverage decreased by 0.001%, not sure how to fix this. I am happy to get your comments and suggestions.

@dougwilson

dougwilson commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

Thanks @sboehler! The change is now very big, so it will take quite some time before I can review and accept it now, compared to the first PR, so please bear with me for the length of time it will take. The main reason there was a split is because of course by the time _end is called, you cannot set any headers, because they were already sent, so the logic that had to do with setting Set-Cookie header was there and saving the session was in the other location. In addition, the API for headers is a sync APi so it can only perform sync operations while the writing & end are async and can perform async operations like saving to the store. I hope that helps understand the split.

@dougwilson

dougwilson commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

In addition, and I don't think it's in the tests, your change must support the altering of res.session between writing the header and ending the response, or we cannot accept until the next major version. An example you may want to test if you haven't already:

app.use(function (req, res) {
  req.session.foo = 'bar';
  res.write('<html>');
  setTimeout(function () {
    req.session.foo = 'baz';
    res.end('</html');
  }, 1000);
});

Should result in foo = baz in the data store, not foo = bar for compatibility with the current implementation.

@dougwilson dougwilson self-assigned this Mar 30, 2016
@dougwilson

dougwilson commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

I also find your code much harder to read, particularly because it's altering _callback and there is a lack of whitespace, and I'm not inclined to accept the new rewrite just in general. I appreciate the effort, but the code needs to make sense and match the style of those of us who are maintaining it.

@sboehler

sboehler commented Mar 30, 2016

Copy link
Copy Markdown
Author

Hi @dougwilson , thanks for the quick feedback.

  • Your example will likely break with my patch, as part of my idea is to start saving the session already 'on headers'. I wasn't aware of the requirement but I see why it is a breaking change. Also, it explains why you only start saving the session on _end, that's one thing I was confused about.
  • White space could be fixed, no problem. As for _callback, the idea is to separate the _end logic from the session commit - I needed some way to transfer context from _end to the outside. While _end is being executed, we discover the task at hand and hence the context and the task at hand might change, therefore the reassignment.

The easy fix for #276 (which makes the 'rolling' option useless in practice in my use case) could then the initial proposal, touching the session twice (or some way via a global which remembers whether the session has been touched). Could you live with that? My principal ambition is that the behavior of rolling is restored, the other refactoring has been an insightful and fun exercise.

Thanks for your work with express, I appreciate it a lot.

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

Reviewers

No reviews

Assignees

@dougwilson dougwilson

Labels

pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sboehler @dougwilson @gabeio @cianclarke @chill117