session icon indicating copy to clipboard operation
session copied to clipboard

Simplify `res.end()` proxy logic — current implementation adds complexity with little real-world benefit

Open raphendyr opened this issue 8 months ago • 3 comments

Problem

The current implementation proxies res.end() in a way that tries to ensure the session is saved before completing the response. However, this logic is complex, fragile, and not reliable in real-world usage, particularly with redirects and concurrent browser requests.

Background

The proxy behavior works roughly like this:

  1. If data is passed to res.end(), it writes all but the last byte.
  2. res.write() triggers res.writeHead(), buffering headers.
  3. Data is buffered and sent if large enough.
  4. Concurrently, session.save() or session.touch() is called.
  5. The save/touch callback finally sends the last byte via res.end().
  6. Client receives the last packet and considers the response complete.

This seems designed to ensure session data is saved before the client makes another request. It may help in tests, but it breaks down in browsers for several reasons:

  • Browsers act on redirects immediately after receiving headers, as redirect doesn't contain a body by the spec.
  • Browsers can issue multiple concurrent requests, even to the same domain, which defeats the assumption of serialized communication (this was already the case in 2009).
  • With HTTP/2 and HTTP/3, multiplexed streams make the idea of strict request ordering obsolete. While those aren’t yet supported by this library (or node), they would likely require different solutions altogether.

In practice, this complexity doesn't prevent session races and makes the code in the library error prone and harder to maintain.


Example Workaround

Today, to ensure a session is saved before a redirect, we have to do this manually:

app.post('/login', (req, res) => {
  req.session.user = 'user';
  req.session.save(() => {
    res.redirect('/');
  });
});

This workaround works, but it exposes the fact that the res.end() proxy doesn't reliably help in the redirect case, which is likely one that users were hoping to be solved.


Proposal

I propose removing the res.end() proxy entirely and replacing it with much simpler logic.

Option 1: Delay header send

const writeHead = res.writeHead;
res.writeHead = function (...args) {
  req.session.save((err) => {
    if (err) throw err;
    writeHead.apply(this, args);
  });
};

This ensures that the session is saved before headers are sent. Because res.write() and res.end() call this from within their internal flow, we still need to add small proxies there, since we cannot delay execution without async/await. See #704 for some details.

As a benefit, errors can be thrown before the response is committed and thus should result 500 as the response.

Option 2: Async session save

const end = res.end;
res.end = function (...args) {
  req.session.save((err) => {
    if (err) console.error(err); // Better handling should be added
  });
  end.apply(this, args);
};

This approach decouples session saving from the response entirely. While there's a risk the session isn't saved before the next request, that’s already possible today. More importantly, this avoids unnecessary complexity.

Some of the session races condition could be mitigated or logged in the store (e.g., compare and swap or diff-based updates), which are conceptually impossible in this library.


Why This Matters

  • The current implementation is complex, difficult to reason about, and not reliable.
  • Users have problems with the redirect, as seen in #360. Also fix attempts in PR #69 and PR #157.
  • Issue #477 and PR #751 show how current solution has incomplete function signature. In addition, the fix would make the code even more complex.
  • Modern browsers don't behave in the way this logic expects.
  • Removing this complexity would make express-session easier to use and maintain, while encouraging explicit and predictable session handling by developers.

Conclusion

I believe we should remove the complex res.end() proxy logic. In addition, we should document best practices clearly (e.g., calling session.save() manually before redirects). I'm happy to help with an implementation or documentation update if this direction sounds good.

What do you think?

Do you find errors in my understand on reasoning?

raphendyr avatar Apr 29 '25 21:04 raphendyr

Overall, your logic seems good, i still need to think about it a bit more, and if your proposal works correctly, then we would fix two small bugs

bjohansebas avatar May 01 '25 03:05 bjohansebas

@raphendyr Do you want to send a PR to try it out?

bjohansebas avatar May 16 '25 02:05 bjohansebas

Sure. I look into implementing something during the next week (probably).

raphendyr avatar May 16 '25 16:05 raphendyr