Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raphendyr opened this issue Apr 29, 2025 · 3 comments

Comments

@raphendyr
Copy link

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


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 raphendyr added the bug label Apr 29, 2025
@bjohansebas
Copy link
Member

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
Copy link
Member

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

@raphendyr
Copy link
Author

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

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

No branches or pull requests

2 participants