Our middleware system sucks #10

Closed
opened 2022-05-16 16:34:28 +00:00 by bvisness · 1 comment
Owner

Our middleware system is bad and I don't like it.

So, when we were developing the site, we were dissatisfied with the approaches to "middleware" that we have seen in other web frameworks. I've attached a document that I wrote during development that contrasts two major approaches (neither of which is used purely in the wild). The fundamental problem, as I see it, is that the behaviors people put in "middleware" are torn between two major use cases:

  • Doing things in sequence - for example, checking auth before the main handler, or logging errors after the main handler.
  • Wrapping existing handlers - for example, a middleware that times how long a request took to process.

The most popular design for middleware these days is a "composition" approach where you have a generic "handler" function signature, and everything is achieved by wrapping handlers inside handlers. A pure function composition approach might look like:

function authMiddleware(next) {
  return function(req, res) {
    // fetch user info from cookie or whatever
    if (!currentUser) {
      req.status(401).send("Unauthorized")
    }
    
    next(req, res)
  }
}

function timingMiddleware(next) {
  return function(req, res) {
    const before = getTime()
    next(req, res)
    const after = getTime()
    
    console.log(after - before)
  }
}

routes.GET("/foo", authMiddleware(fooHandler))
routes.GET("/bar", timingMiddleware(authMiddleware(barHandler)))

const adminRoutes = routes.Group("/admin", handler => timingMiddleware(authMiddleware(handler)))
adminRoutes.GET("/users", usersHandler)
adminRoutes.GET("/metrics", metricsHandler)

I'm not really sure how much I like explicitly writing out function composition in this way. It's accurate to how things are handled, though. I feel like this doesn't do a great job expressing the "do things in sequence" nature of many middlewares, but things like the timingMiddleware obviously need to wrap the entire next handler.

Other frameworks typically allow you to provide these handler functions as a list instead, and the framework essentially converts the list to function composition:

function authMiddleware(req, res, next) {
  // fetch user info from cookie or whatever
  if (!currentUser) {
    req.status(401).send("Unauthorized")
  }
    
  next() // calls back into framework
}
function timingMiddleware(req, res, next) { ... }

routes.GET("/foo", authMiddleware, fooHandler)
routes.GET("/bar", timingMiddleware, authMiddleware, barHandler)

const adminRoutes = routes.Group("/admin", timingMiddleware, authMiddleware)
adminRoutes.GET("/users", usersHandler)
adminRoutes.GET("/metrics", metricsHandler)

However, this results in unclear control flow and stack traces that I really do not like:

framework.handleRequest("/bar")
timingMiddleware()
framework.next()
authMiddleware()
framework.next()
barHandler()

This indirect function composition makes debugging more difficult and lots of things more cumbersome, and I don't want to do it.

We explored the idea of all middlewares being a simple list, e.g.

const adminRoutes = routes.Group("/admin",
  // "Before" handlers
  [timingMiddlewareStart, authMiddleware],
  // "After" handlers
  [timingMiddlewareEnd],
)

but this clearly splits up many middlewares in a really awkward way and generally sucks.


So - is there a way to make the composition approach work for us? We currently use something like the example I gave, except that we attempted to tame some of the seemingly unnecessary nesting by making middlewares that simply call a sequence of utility functions. This has turned out to suck major ass: a147cfa325/src/website/routes.go (L294)

Our middleware system is bad and I don't like it. So, when we were developing the site, we were dissatisfied with the approaches to "middleware" that we have seen in other web frameworks. I've attached a document that I wrote during development that contrasts two major approaches (neither of which is used purely in the wild). The fundamental problem, as I see it, is that the behaviors people put in "middleware" are torn between two major use cases: - Doing things in sequence - for example, checking auth before the main handler, or logging errors after the main handler. - Wrapping existing handlers - for example, a middleware that times how long a request took to process. The most popular design for middleware these days is a "composition" approach where you have a generic "handler" function signature, and everything is achieved by wrapping handlers inside handlers. A pure function composition approach might look like: ```js function authMiddleware(next) { return function(req, res) { // fetch user info from cookie or whatever if (!currentUser) { req.status(401).send("Unauthorized") } next(req, res) } } function timingMiddleware(next) { return function(req, res) { const before = getTime() next(req, res) const after = getTime() console.log(after - before) } } routes.GET("/foo", authMiddleware(fooHandler)) routes.GET("/bar", timingMiddleware(authMiddleware(barHandler))) const adminRoutes = routes.Group("/admin", handler => timingMiddleware(authMiddleware(handler))) adminRoutes.GET("/users", usersHandler) adminRoutes.GET("/metrics", metricsHandler) ``` I'm not really sure how much I like explicitly writing out function composition in this way. It's accurate to how things are handled, though. I feel like this doesn't do a great job expressing the "do things in sequence" nature of many middlewares, but things like the `timingMiddleware` obviously need to wrap the entire next handler. Other frameworks typically allow you to provide these handler functions as a list instead, and the framework essentially converts the list to function composition: ```js function authMiddleware(req, res, next) { // fetch user info from cookie or whatever if (!currentUser) { req.status(401).send("Unauthorized") } next() // calls back into framework } function timingMiddleware(req, res, next) { ... } routes.GET("/foo", authMiddleware, fooHandler) routes.GET("/bar", timingMiddleware, authMiddleware, barHandler) const adminRoutes = routes.Group("/admin", timingMiddleware, authMiddleware) adminRoutes.GET("/users", usersHandler) adminRoutes.GET("/metrics", metricsHandler) ``` However, this results in unclear control flow and stack traces that I really do not like: ``` framework.handleRequest("/bar") timingMiddleware() framework.next() authMiddleware() framework.next() barHandler() ``` This indirect function composition makes debugging more difficult and lots of things more cumbersome, and I don't want to do it. We explored the idea of all middlewares being a simple list, e.g. ```js const adminRoutes = routes.Group("/admin", // "Before" handlers [timingMiddlewareStart, authMiddleware], // "After" handlers [timingMiddlewareEnd], ) ``` but this clearly splits up many middlewares in a really awkward way and generally sucks. --- So - is there a way to make the composition approach work for us? We currently use something like the example I gave, except that we attempted to tame some of the seemingly unnecessary nesting by making middlewares that simply call a sequence of utility functions. This has turned out to suck major ass: https://git.handmade.network/hmn/hmn/src/commit/a147cfa3259a4aee89f070bfd7e44bf8c78c4e30/src/website/routes.go#L294
bvisness added the
hmmmm
label 2022-05-16 16:40:43 +00:00
bvisness added a new dependency 2022-05-21 22:52:57 +00:00
Author
Owner

This now uses a simple composition approach. My concerns about confusing stack traces were unfounded, this handles the majority of middleware cases just fine, and more advanced cases can always just process handlers differently. No major downsides that I see right now.

I guess the common practices in the industry were pretty good this time.

This now uses a simple composition approach. My concerns about confusing stack traces were unfounded, this handles the majority of middleware cases just fine, and more advanced cases can always just process handlers differently. No major downsides that I see right now. I guess the common practices in the industry were pretty good this time.
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: hmn/hmn#10
No description provided.