FR: Ability to always call next() about serve-static

[dougwilson]

Can you clarify on when you expect it to be called, exactly? After the response has been sent? But of course what about when the client aborts, when should it be called? Need more information on the expected timing of the call.

[kwasimensah]

My current version. I deal this would be replaced by serveStatic adding the listeners.

const serveFromDistImages = express.static(path.join(__dirname, "dist", "images"), {
    immutable: true,
    index: false,
    maxAge: 1000 * 60 * 60 * 24, // 24 hours
 });

app.use((req: express.Request, res: express.Response, next: express.NextFunction) => {
    let hasCalledNext = false;
    function callNext() {
      if (!hasCalledNext) {
        hasCalledNext = true;
        next();
      }
    }
    function catchPipeEvent() {
      res.on("close", callNext);
      res.on("finish", callNext);
    }
    res.on("pipe", catchPipeEvent);
    serveFromDistImages(req, res, next);
    //res.removeListener("pipe", catchPipeEvent);
  });

[kwasimensah]

The use case I'm trying to catch is when the client aborts. I found in my unit testing it's actually hard to get express/https.server to shutdown cleanly i.e. if the client aborts while the request is asynchronously talking to the database, https.server.close won't wait for that to finish/won't wait for any in progress async middleware to finished before the "close" callback is called.

I'm trying to add middleware at the beginning and end of each request to track what's still in progress independent of the connection being severed so I can delay fully closing if there's any middleware that might still use external resources.

[dougwilson]

I think that there is no way to support what you want, as it breaks the Express routing mechanism. The reason fallthrough option calls next() is because this middleware didn't send a response. But once a middleware writes a response, it cannot just call next() and continute routing -- routing has ended at that point.

Based on your request, one would hope that Node.js would provided such APIs to support you, but alas, they leave many APIs to user-land. One such user-land module that would provide your solution is https://www.npmjs.com/package/on-finished . Simple example based on your use-case:

const inProgress = new Set()
const onFinished = require('on-finished')

// add as probably your first middleware
app.use((req, res, next) => {
  inProgress.add(res)
  onFinished(res, () => inProgress.delete(res))
})

And if you really want this module to always call next() (which is highly recommend against, and the routing system on Express is not designed to be re-entered after a response is started), here is an example wrapper you can use:

const once = require('once')
const onFinished = require('on-finished')

// takes place of your serveStatic middleware call
const serveSomething = express.static(...)
app.use((req, res, next) => {
  const done = once(next)
  onFinished(res, () => done)
  serveSomething(req, res, done)
})

[kwasimensah]

So on-finished isn't what I'm looking for because if the client closes the connection in the middle of a request using async middleware "close" will be fired before all the middleware has finished.

I'm trying to find official expressjs documentation saying it's unsafe to process middleware after sending the response (as long as you don't try to edit anything about the request object). I just realized I got the idea from this non-official article I know saying "it works for me" is not a sign that it's a supported use case but it is working so far.

Should this be resolved by:

1. adding explicit wording in the documentation saying calling next() after end() is wrong and it should throw an error. I know I'm being pedantic but the docs currently say

If the current middleware function does not end the request-response cycle, it must call next() to pass control to the next middleware function.

without being specific about about what's considered the full request-response cycle.

2. making this a supported use case for people who don't want to block sending a response for server specific work tied to the middleware chain (and not specifically the connection) and adding tests to spot regressions.

3. adding a dummy middleware that just calls res.end() and acts like 2) but with a delay and using #136 (comment) for serve-static.

I'm a fan of 2. The only other option I see is adding checks after every promise in middleware to see if the request is active but that doesn't catch issues if the request is closed mid-await.

[dougwilson]

So some of your comments seem outside the scope of this particular module (regarding Express's docs or Express's router operations), so this module's issue tracker is not a great place to have such a discussion since it wouldn't be visible to the relevant parties.

Let's keep the conversation scoped to this module, since that is where this issue is opened :) If on-finished is not what you're looking for, I really have no idea what you want, then? Perhaps a pull request is in order for the changes you're looking to be made to this module so I can better understand?

[kwasimensah]

Moved the thrust of my point to expressjs/express#4356. Coming up with a pull request now

[dougwilson]

Thanks @kwasimensah ! Please also ensure you include tests that test the specific behavior you keep describing where the client aborts mid request. I would love to see that, as that is something that the on-finished module specifically handles, so I'm a bit perplexed on how it does not solve your problem, especially since it seems to match exactly what #136 (comment) is doing as far as I can tell.