Express: Express 'cannot GET' will cause an http headers sent error in app.use

[RonanQuigley]

So this might not be an issue with express' internal workings, as I did post this up on stackoverflow initially thinking that this issue was purely arising from how I am approaching using express, but after experimenting with multiple setups I'm not so sure what's what. Currently I have a setup for hot reloading as follows:

// server.js
const app = express();

app.use(function (req, res, next) {
    require('./server/index')(req, res, next);
    require('./server/foo')(req, res, next); // another url page
});

Each require file inside app.use follows the following example template:

const router = express.Router();

router.get('whatever endpoint' you want, (req, res) => {
  console.log('whatever endpoint you want');
  res.send('whatever message you want');
});

module.exports = router;

My understanding is that app.use with just a callback will apply to every possible route. And if a user navigates to a route that doesn't exist, say '/bar' for example, express' cannot GET/ should be displayed at whatever the incorrect endpoint was.

What actually happens however, is that express crashes with the ERR_HTTP_HEADERS_SENT error. So of course that's bad, so to get around that, I would just apply my own catch-all inside the above app.use at the end of the function as follows:

//server.js
app.use(function (req, res, next) {
    require('./server/index')(req, res, next);
    require('./server/foo')(req, res, next); 
    require('./server/catch-all')(req, res, next);
});

//catch-all.js
router.get('*', (req, res) => {    
  console.log('catch all');
  if(!res.headersSent){ // this is necessary to prevent the ERR_HTTP_HEADERS_SENT error
    res.redirect('/');
  }
})

This fixes all of the issues, but I've noticed that the catch-all console.log is always called twice; once for when the rest of the require statements have been called and it has not found a match, hence using the catch-all endpoint. The second is for when the redirect to the main page happens.

If my understanding is correct, since app.use() is being called for every single request, this causes the double call effect when invoked by the redirect handler? If so, if we omit the catch-all, and go back to what was originally done at the top of this post, why does express crash with the ERR_HTTP_HEADERS_SENT error? Is it again because of the app.use being called for every route, including routes that aren't defined and being handled by cannot GET?

If this is the case, and this is the crux of what I'm trying to get at, shouldn't express internally recognise that the cannot get / error handler has already been called? It seems like it's attempting to send two responses that both say cannot GET whatever page the error is on, hence the ERR_HTTP_HEADERS_SENT error.

The alternative is splitting the above into two separate app.use calls, that _only_ handle specific routes:

app.use('/', require('./server/index'));
app.use('/foo', require('./server/foo'));

However, if you have numerous routes, having loads of app.use calls littering your code becomes repetitive, and your docs state both options as being valid. Is it just a case of it needing to be done this way by design or am I perhaps missing another possible option?

[dougwilson]

I'm trying to following and definitely want to fix whatever the issue is, but not sure based on the description what is happening. Is it possible you can provide an example app that I can simply copy and paste and run and make a request to to see the issue your describing?

[RonanQuigley]

Sure, so I'm not at my work machine at the mo, but here is the initial project I used for testing the problem. I can add in the missing code from my original post in if you're still confused:

https://github.com/RonanQuigley/express-chokidar-hot-reload

I should add: if you see return next('router') you can just remove it. I realised that was nothing to do with the problem yesterday. I'll update the repo if need be once I'm back

[dougwilson]

Yea, please update it so we can be on the same page 👍 when you get back, ideally the following will help as well:

1. Version of Express.js
2. Version of Node.js
3. Instructions for running your code (if any besides node file.js
4. What HTTP request to make to the app (like GET /) to see the issue

Thanks, again!

[RonanQuigley]

Okay if you check out the repo I previously posted I've updated it. Details you requested are in readme file too. If it's a non-issue and just a case of me misunderstanding how express works, that's okay. Either way, would be good to know what's up so I'm on the right track.

[dougwilson]

Thanks @RonanQuigley ! I cloned and believe I followed the directions, but I'm not seeing any error? Here is the steps I did:

1. Clone the repo
2. npm install
3. npm start
4. curl http://127.0.0.1:3000/

The server running just logs the following out:

index

And the curl just outputs the following:

index - change this text to something else and refresh the browser to see the result

Let me know what I'm missing, happy to take a look!

[RonanQuigley]

Did you look through the server.js file comments to see each implementation scenario? Doing just a curl for the index won't show you the problem. If you curled to say http://127.0.0.1:3000/bar it would crash though. I'm assuming you used a browser for testing as well?

[dougwilson]

Sorry, there is a lot of things and I was thinking from above you were saying the instructions were just in the README, which says to install, start, and call GET / so I did so, haha. I don't have a lot of time any more, so I'll try to circle back around to this tomorrow 👍I didn't try anything you didn't really say, because I don't know what to do without some instructions, so didn't try a browser, either, since nothing I saw in here or the README said to use a web browser, very sorry :(

I'll look through that server.js tomorrow, but if it's possible between then so we don't end up in the same situation, is there a canonical place you can just list step-by-step exactly what I need to do after cloning to reproduce the issue?

[RonanQuigley]

But the instruction is in the readme file:

"open server.js at the root of the project and follow the commenting for a guide of the issue."

Perhaps wires have been crossed here? Either way, the canonical place is to use the server.js file. It's heavily commented with each step for this purpose. If that wasn't clear enough, my bad.

[dougwilson]

It's no problem, really sorry because I only had a small window at a computer to look into it and not much time so tried to just follow it. I'm back to only a phone again which is why I cannot look into it. Also, maybe someone else will be able to look into it in the meantime as well before tomorrow.

[dougwilson]

@wesleytodd in case he has any time to check it out and hasn't seen the issue as well :)

[RonanQuigley]

Hey, just wondering if you had a chance to look into the issue yesterday?

[wesleytodd]

Hey, for some reason I missed that @ mention, sorry.

There are a few things wrong with your initial example. By calling multiple chains of middleware like you are, it will call them in parallel, and if any have async operations you will not get what you want. What you should do is:

app.use(require('./server/index'), require('./server/foo'));

This will only run foo after index completes. Is that the answer you needed?

[RonanQuigley]

Not a problem!

Ah right, makes sense. So I gave this a shot, and whilst it means you can just have one app.use call and don't get the ERR_HTTP_HEADERS_SENT error, you also lose the ability to hot reload your server when changes occur through chokidar. Why I'm not so sure (maybe the use of (req, res, next) ?), so perhaps the only way to get around this is to have an app.use for development and one for production like the one you listed? i.e:

// development
app.use(function (req, res, next) {
    require('./server/index')(req, res, next);
    require('./server/foo')(req, res, next); // another url page
});

// production
app.use(
require('./server/index'), 
require('./server/foo')
);

Ideally I'd look to just have one for both, so that I didn't have to repeat code, but if that's not possible with express, unless you have any other ideas, this would probably be the best way forward.

[wesleytodd]

Ok, I have no idea what your hotreloading stuff would do, but I guess it is klobers require, so you can re-require on every request and get the new updates. So you can still do this, (although you should just be restarting the process on changes instead because it is MUCH more predictable) by doing something like this:

app.use(function (req, res, next) {
    require('./server/index')(req, res, function (err) {
        if (err) return next(err)
        require('./server/foo')(req, res, next); // another url page
    });
});

The reason you are getting the headers thing is because of the multiple middelware chains.

[dougwilson]

That makes a lot of sense @wesleytodd . Yea, I see that too digging in: in one middleware, there are two require and immediately invoke their returned function, which means if the first one wrote a response, the second one will also try to write a response, triggering the headers already sent error. Of course if the hotreloading is doing the require overwrite like you think, the app.use(require('./server/index'), require('./server/foo')); wouldn't work as observed. Perhaps the best way around this is to write a little function to encapsulate the requireing:

app.use(dynamic('./server/index'), dynamic('./server/foo'));

function dynamic(lib) {
  return function (req, res, next) {
    return require(lib).apply(this, arguments)
  }
}

That would then allow you to write out what looks like standard Express syntax but get it to require the lib path on every request 👍

[wesleytodd]

That is a good solution given the context. But to @RonanQuigley, I would caution you that you are entering murky waters, and should probably re-asses your setup.

Take a look at nodemon or some other solution which reloads the whole process, because hot-reloading in place is really error prone and will cause you more trouble than it is worth in my experience.

[RonanQuigley]

That does look like a good solution. It's late here, but I'll give this a shot tomorrow morning.

Actually, I originally did have nodemon setup for this, but the dev efficiency for it (at least with how I had it setup anyways), meant that if I wanted changes to be reflected in the browser, I'd have to restart express. And if it's say, css changes you're looking to do on server side, then it's a complete pain to have to wait n number of seconds between each save for a restart to occur. Multiply that by potentially 100's of times a day and your dev efficiency tanks. The current setup that I've shown eliminates that issue completely, allowing for changes to be reflected immediately. I'm all ears though if you think that there is a better way, though of course that's prob beyond the scope of express issues.

[wesleytodd]

If it works for you, then go for it! Productivity is all that really matters. But I would guess you probably spent more time opening and dealing with this issue than you will save in the lifetime of your project from not having to manually restart your server ;)

[RonanQuigley]

Hey, sorry for the delay in getting back.

Just to say that the solution @dougwilson posted up works great! Thanks for the help with it, feel free to close this up now 👍