Skip to content
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

Sending after request end doesn't throw error, hangs response instead #766

Closed
josh-byster opened this issue Jul 6, 2020 · 5 comments · Fixed by #767
Closed

Sending after request end doesn't throw error, hangs response instead #766

josh-byster opened this issue Jul 6, 2020 · 5 comments · Fixed by #767
Labels

Comments

@josh-byster
Copy link

josh-byster commented Jul 6, 2020

Hi, I've noticed that when including express-session, the res.end behavior (from what I'm assuming is due to the proxied res.end) deviates from what I would expect in handling errors.

Generally if I were to call res.end and then later inadvertently call res.send after that (perhaps in another middleware handler), I would receive an ERR_HTTP_HEADERS_SENT. That's what I was expecting to see—however, instead, if I include the session middleware, the headers send but it waits for 5s (which I think is the default keepAlive timeout) until erroring out with a content length mismatch (since it is setting Content-Length but the body doesn't go through). In my opinion, it would be helpful to have that original error bubble through if possible, since it was not clear that including this middleware would cause this API change to res.end as a side effect.

var express = require("express");
var app = express();

// Uncommenting the middleware usage leads to different functionality in handling the duplicate response sending
app.use(
  require("express-session")({
    secret: "keyboard cat",
    resave: true,
    saveUninitialized: true,
  })
);

app.get("/", function (req, res) {
  res.end();
  // Doesn't throw error! Waits for keepAlive duration
  res.send("This is a test!");
});

app.listen(5000);
@josh-byster josh-byster changed the title Sending after request end doesn't throw error Sending after request end doesn't throw error, hangs response instead Jul 6, 2020
@dougwilson
Copy link
Contributor

Thank you, I can reproduce that behavior. It seems specific to there being no body to your first res.end. If you change it to something like res.end('foo') I believe it does the behavior you are expecting. Can you confirm that?

@josh-byster
Copy link
Author

josh-byster commented Jul 7, 2020

@dougwilson Thank you for the quick response on this and for investigating! From what I can observe, yes, I can confirm your statement is true.

For more of a concrete insight on how this might manifest (as I agree, it's rather specific), this confusion surfaced when I ran into this while integrating PassportJS—I set up express-session in conjunction with Passport. In the authenticate middleware, Passport calls the no-body res.end(), therefore any middleware that inadvertently writes to the response afterwards (which I had done) may be prone to having the same issue occurring. I had assumed if my middleware were writing to the response after Passport, I would see the ERR_HTTP_HEADERS_SENT surfacing, but since I just got that hung response with no error message, I didn't think to look for middlewares overwriting responses in that way.

@dougwilson
Copy link
Contributor

Hi @josh-byster , sorry if my response was confusing: I'm not trying to say this is not a bug, I just wanted to make sure that the simple fix would actually result in the behavior you're looking for by having you confirm if that behavior is what you're looking for. Then we would fix the specific case that is not resulting in said behavior to make sure your case is working.

@josh-byster
Copy link
Author

Completely understandable, and thank you for the clarification :) I just wanted to give an example of a concrete use case in addition, as I realized my original issue report lacked a concrete way for which this behavior may surface.

@dougwilson
Copy link
Contributor

Hi @josh-byster there is a PR #767 that should fix it if you are interested in giving it a test.

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

Successfully merging a pull request may close this issue.

2 participants