Why no throwing from middleware?

[stuartpb]

I have four questions:

• How much of a performance hit is it to catch errors in middleware?
• Doesn't this mean that an error in a route will bring down the entire server?
• Is there any way a user can re-introduce catching in middleware, short of wrapping every middleware function in a try/catch?
• What about uncaught promises?

[lukeed]

1. Are you talking about try/catch inside the middleware loop? Otherwise, errors are caught if you pass them inside next(): https://github.com/lukeed/polka/blob/master/lib/index.js#L61

2. No, not if you used the method described. You should always be taking responsibility for your own errors though, even if a try/catch makes its way into the core loop.

3. Not for try/catch right now. You can wrap every middleware with that or a Promise.

4. Like with (2), you're always responsible for your own errors. Polka is just a router that invokes some decorator functions.

FWIW, I'm refactoring right now, before 1.0... The try/catch & ability to throw from within middleware is still on the table.

[stuartpb]

Yeah, I'm talking about the ability to throw from middleware. On one hand, I could believe that not having it would entail a measurable performance improvement; on the other hand, not having it means that your server can potentially come down over anything.

Now that I think about it, I guess, if a polka app is itself callable as middleware (as is the case with Express, and I'm pretty sure Connect before it), you could probably introduce all this by having a layer around your app that hands off the request within a try/catch (or, in the case of Promise rejections, wrapping it in a Promise that resolves at the end of the middleware - which would also catch synchronous throws).

[lukeed]

Yeah, definitely. There's an HTTPS example I added that shows how a Polka app can be embedded.

With the most recent Loop changes, I haven't measured the performance impact, so I could try now. But before, it was pretty bad!

[stuartpb]

I'm thinking what would make sense for implementing catching, if it really is that much of a burden in production (and I know it certainly can be, due to all the stack-trace baggage the system otherwise doesn't have to maintain), would be to add a method to Polka that looks something like this (without looking at the implementation - the internals would probably allow for something lighter weight than instantiating a new instance just to use itself as middleware):

Polka.prototype.protected = function protected(active) {
  if (active) return polka().use((topErr, req, res, next) => {
   new Promise((resolve, reject) =>
     this(topErr, req, res,
       err => err ? reject(err) : resolve())
     ).then(next, next);
  }) else return this;
}

This would mean that an app could add a call like .protected(process.env.NODE_ENV=='development') after any routes it wants to instrument in development, and in production, the instrumentation would be skipped.

It would also let the app have a more granular instrumentation policy, applying .protected to individual routes based on a more complex configuration.

[stuartpb]

Actually, I just looked, and Polka neither exposes itself as a function (which is what I meant by "allows itself to be used as middleware") nor exposes a .handle method (which is what the function call is an alias for).

Here's a maybe clearer way to express it, if .handle were implemented (I removed the topErr as, on second look, even Express apps don't support being called as error-handling middleware):

Polka.prototype.protected = function protected(active) {
  if (active)
    return polka().use((req, res, next) => {
     new Promise((resolve, reject) =>
       this.handle(req, res, err => err ? reject(err) : resolve())
     ).then(next, next);
    });
  else return this;
}

[lukeed]

If I were to go a route other than try/catch, I'd do something like this:

function onError(req, res, next, err) {
  req.pathname.includes('foo') && next(); // ignore
  console.log(`> ERROR: ${req.method}(${req.url}) ~>`, err);
  res.statusCode = err.code || 500;
  res.end(err.toString() || 'Oops!');
}

polka({ onError }).use(...);

//=> internally

let next = err ? this.onError(req, res, next, err) : loop();

[lukeed]

I might even go that route anyway & just call the opts.onError from within the (atm, hypothetical) catch block.

I'll have time this weekend to flush out more of this refactor, and I'll be considering this. 👍

[stuartpb]

Okay, but:

1. catch doesn't catch unhandled Promise rejections - only synchronous throws. That's why the wrapper I wrote handles the prior stack with a Promise: it'll catch error values that are synchronously thrown or asynchronously rejected.
2. Given that you were saying just catch introduced a lot of overhead in earlier testing, it would seem you'd want to make this an optional wrapper.

Also, do forgive my impertinence, but are you not aware that quaternary error-handling middleware functions are the established format for handling errors in the Express ecosystem?

[lukeed]

It's not Polka's job to clean up everyone's mess. You have to catch your own crap, as dirty as it may be. If you're not catching your own Promise rejections, you're either writing the middleware incorrectly or just setting yourself up for failure, regardless of the server framework used.

That link gives the same advisory note that I do; but their is less noticeable:

Notice that when not calling “next” in an error-handling function, you are responsible for writing (and ending) the response. Otherwise those requests will “hang” and will not be eligible for garbage collection.

Nothing on that front changes when using sync or asynchronous functions.

At this point, I really don't care about throw to be honest. There are 3 other ways to throw & handle errors, all of which were actually the primary method with Express, too.

The primary goal of Polka is to be a familiar API, not exact, and perform as close to native http as possible, while also offering routing out of the box. Express compatibility is not a goal -- it's just a nice-to-have. An identical API would almost certainly perform at Express-like numbers.

If throw decreases the req/s by even 1k, it's not worth it. It's better to (re)write the middleware in compliance, the way it should have been.

[lukeed]

For example, this is wrong regardless of server choice:

const sleep = ms => new Promise(r => setTimeout(r, ms));

app.use((req, res, next) => {
  sleep(5e3).then(_ => {
    console.log('> ok, NOW, im done');
    next();
  });
  next();
})

[Deleted user]

@stuartpb each middleware should handle its own errors but if you are that crazy for throwing you can implement something like this

const http = require('http');
const polka = require('polka');

const app = polka({ createServer: false });

const server = http.createServer((req, res) => {
  try {
    app.handler(req, res);
  } catch (err) {
    console.error(err);
    app.send(res, 500, err.message);
  }
});

server.listen(8080);

createServer option will work after #13 gets merged

@stuartpb if you want it so bad to be internal you could start new issue on having similar wrapper like shown above in npm module @polka/safe

[lagden]

hey @lukeed

I'm koa user and I am doing a lab with polka...

One thing that I fell missing: Put middleware in specific route

polka()
	.get('/', home)
	.use(bodyparser)
	.post('/data-json', dataJson)
	.use(auth, bodyform)
	.post('/data-form', dataForm)
	.listen(3000).then(_ => {
		console.log(`>>> Running on 3000`)
	})

This way, all routes will pass through the bodyparser and sometimes it is not necessary!

Could be:

polka()
	.get('/', home)
	.post('/data-json', bodyparser, dataJson)
	.post('/data-form', auth, bodyform, dataForm)
	.get('/another', another)
	.listen(3000).then(_ => {
		console.log(`>>> Running on 3000`)
	})

Regards!!

[lagden]

@lukeed never mind... 👆
https://github.com/lukeed/polka#usebase-fn

[lukeed]

Hey, welcome!

Ya theres that, but also this issue #18

I am leaning towards having this in 0.5, pending no performance hits.

[lukeed]

Closing this as it's an older discussion & still stands by my reasons for excluding it. Thank you though! 🙏

[lukeed]

Will be included in the 1.0 push for Express compatibility – and especially because the try/catch performance hit has been removed in Node 10.x and onwards. 👍

[akotlar]

Great work on this lib, thanks!

[neves]

Today, what's the correct way to avoid the app crash if some error do not get catch?
I tried this solution, but doesn't work: #12 (comment)

[lukeed]

@neves At this point I'd use polka@next, but if you needed to stick with the current stable, this is how it'd be done:

const polka = require('polka');

const app = polka().get('/', (req, res) => {
	throw new Error('Hello');
});

// Redefine `app.handler` w/ try-catch
// You only have to do this on the MAIN/TOP application!

const old = app.handler;
app.handler = function (req, res, info) {
	try {
		old.apply(app, arguments);
	} catch (err) {
		console.log('> I caught error:', err);
		res.end('Oops~');
	}
}

app.listen(3000);

[neves]

polka@next works like a charm, with one little problem. If I instantiate a new polka inside a middleware (sup-apps) I need to define onError again, but the expected behaviour was to bubble the exception to the main/top application. Can you confirm that?

[lukeed]

No, every Polka instance has a default onError handler. If you want to customize it, it has to be defined.

It's best to define your own router file/package and then import that where needed. I do this in all larger applications.

// router.js

import polka from 'polka';

function onError(err, req, res, next) {
  // Custom...
}

export default function () {
  return polka({ onError });
}


// users.js
import router from './router';

router()
  .use(...)
  // Etc

[neves]

Great. That should work.
How are you using ES6 in node for server side?
Babel? flags? "type": "module" in package.json ?
Can you share your deploy strategy?

[lukeed]

Eventually I'll have a full Polka starter kit, but I'm just using Rollup to build.

You can also just do everything above using require and module.exports – no difference :)