Default responder should not console.error 4xx errors

[dougwilson]

Right now the default handler (last step) will console.error when it gets an err, otherwise 404. I think that it probably shouldn't console.error when the err it got was a 4xx error, mainly because a 4xx is supposed to be a client error, of which the server admin probably cannot do anything about. Adding a custom error handler could, of course, let the user log 4xx error all they wanted--this is just about the default behavior. It could probably wait until express 5, though.

[Fishrock123]

+1

[aredridel]

:+1: 5.0

[ritch]

I agree this should be removed. In a hello world express app, you'll sometimes get a nasty error printed when the browser tries to grab /favicon.ico which is pretty annoying.

It could probably wait until express 5, though.

We should avoid removing behavior (however lame it might be) in a minor version. Anything landing in 4.x should be additive or fixes until 5. This change may actually be breaking, even though it seems like it wouldn't. Eg. someone might have a log parser that relies on this error, who knows.

[dougwilson]

Anything landing in 4.x should be additive or fixes until 5. This change may actually be breaking, even though it seems like it wouldn't. Eg. someone might have a log parser that relies on this error, who knows.

Thus the reason I said it should land in 5 :)

[dougwilson]

The reason I only suggested it wait for 5 is because there is a bunch of work around it. If it were to change, it should be made configurable. Of course if it's configurable, it could then easily land on 4 with the default config having it operate as it does now. Standard back-compat stuffs.

[dougwilson]

Also, feel free to read through all the git commits in the history, at least through all the ones over the past couple months. They demonstrate that back-compat pattern well, giving people the ability to opt-in to cool new stuff, but not change the existing behavior for people who do nothing, etc.

[ritch]

The reason I only suggested it wait for 5 is because there is a bunch of work around it.

Makes sense. So would the ideal approach (with a bunch of work) be to land the ability to configure in 4.x and change the default in 5?

[dougwilson]

So would the ideal approach

I already highlighted the ideal approach in the comment below what you quoted :)

[ritch]

Haha not sure how I missed that you said exactly that.... Re-reading my comment it doesn't make any sense. What I'm getting at is that I'd like to take this on and do it the right way.

Can we mark this as 4.x? Or is there anything else to sort out?

[tunniclm]

Would something like this be considered a reasonable back-compatible solution?

diff --git a/lib/application.js b/lib/application.js
index 0ee4def..47e573a 100644
--- a/lib/application.js
+++ b/lib/application.js
@@ -76,6 +76,7 @@ app.defaultConfiguration = function defaultConfiguration() {
   this.set('query parser', 'extended');
   this.set('subdomain offset', 2);
   this.set('trust proxy', false);
+  this.set('log client errors', true);

   // trust proxy inherit back-compat
   Object.defineProperty(this.settings, trustProxyDefaultSymbol, {
@@ -625,6 +626,10 @@ app.listen = function listen() {
  */

 function logerror(err) {
+  if (!this.get('log client errors')) {
+    var status = err.status || err.statusCode || res.statusCode || 500;
+    if (status >= 400 && status <= 499) return;
+  }
   /* istanbul ignore next */
   if (this.get('env') !== 'test') console.error(err.stack || err.toString());
 }

[dougwilson]

@tunniclm, we should focus on getting the needed changed into Express 5.0 instead of adding more weird back-compat solutions. There are already two solutions for Express 4.x users can use for this, and so adding yet another solution is, i.m.o., not very valuable.

Currently the best solution in Express 4.x is to just handle your errors with an error middleware and not forward 4xx ones to the default handler.

As part of the "minimalist web framework" ideology of Express, we historically favor not adding a bunch of configuration switches, as by the time we're done, you may as well just use Hapi.js :)

[tunniclm]

@dougwilson That's fine by me. I assume the next step is to raise a pull request against pillarjs/router then (with a forward-looking change)?

[dougwilson]

Hi @tunniclm, I was hoping the labels would make it clear which mode these issues correspond to :) This is for the finalhandler module and it's already been implemented: https://github.com/pillarjs/finalhandler/commit/0a6847daeaa534df8aeaa7bb3e7d39568648a0a7

[dougwilson]

Sorry, wrong commit; I'll have to take a look later, but yea, this is against the finalhandler module.

[tunniclm]

@dougwilson Arg, so many issues relate to the router I think my brain went on autopilot. Although, I thought this change might be in application.js because I expected that the onerror handler should be called even for 4xx error codes, but just the onerror handler for the default handler shouldn't log them.

Is that incorrect, is the intention that the onerror not be called for 4xx status codes at all (even if it is a user provided function)?

[dougwilson]

Hi @tunniclm, there are really two parts here: a change to finalhandler and then a change in application.js, though that needs to be moved into a new module in pillarjs as well.

The built-in error logging that is implemented in the application.js-split module should use a 1.x version of finalhandler that provides the detected status to the logging function, otherwise all you're doing is duplicating the logic to get the status from the err object, which sucks for maintainability and the ability to reason about the program flow (for example, your patch will still log for err.stats = 201, which is confusing).

So once finalhandler is changed to provide the status code (and yes, it would always be invoked, even on 4xx errors, as it would be invoked whenever there was an err object), then the new module that is built to replace the application.js file would use it and this new logic. This issue is the over-all tracking towards these Express 5.0 features.

I hope that helps! From the previous discussions over a year ago, some of which is written down in the 5.0 pull request in this repo, there are no 5.0 changes that will be in this repository; 5.0 will consist of implementing new features in other modules & extracting all existing functionality into new modules. Implementing new 5.0 features in this repository is in general not working towards the 5.0 goal of extraction.

[tunniclm]

The reason I only suggested it wait for 5 is because there is a bunch of work around it. If it were to change, it should be made configurable. Of course if it's configurable, it could then easily land on 4 with the default config having it operate as it does now. Standard back-compat stuffs.

Just to check vs this earlier comment, are we saying this behaviour should not be configurable in 5?

Also, regarding refactoring parts of application.js out into a new module, it seems like it would be better to do that under an issue specifically for that purpose and refer to that from here.