Move 'doNotWaitForEmptyEventLoop' middleware from 'after' step to 'before'?

[vladholubiev]

Today I added this middleware ⬇️ and I discovered my Lambdas were timing out when they didn't pass validation

middy/src/middlewares/doNotWaitForEmptyEventLoop.js

Lines 1 to 3 in c9c8e70

	module.exports = () => ({
	after: (handler, next) => {
	handler.context.callbackWaitsForEmptyEventLoop = false

It sets false in after step, meaning after handler execution. But if validation fails, handler never runs, and I get a timeout.

I changed after to before and it works as expected now.

Any risk making the same change in the framework itself? Was there a reason to do so specifically?

I mean it's a tiny change, it's faster to submit a PR then writing down this 😅 but who knows, was it by design?

[lmammino]

Thanks @vladgolubev, that's probably a good idea. I think we have to make this middleware slightly smarter to handle this case (and more).

The original idea of having it in the after phase was to make sure that the user does not override it by mistake, but we didn't think it might have affected error cases.

One idea could be to set the callbackWaitsForEmptyEventLoop to false in both phases (before and after) and probably we should do it also in the onError phase.

Also, we could assume the before phase is mandatory and the after and onError phases could be switched off through config.

Using these ideas, this can be a first draft of the new version:

module.exports = (opts) => {
  const defaults = {
    runOnAfter: true,
    runOnError: true
  }

  const options = Object.assign({}, defaults, opts)

  const setFlag = (handler, next) => {
    handler.context.callbackWaitsForEmptyEventLoop = false
    next()
  }

  return ({
    before: setFlag,
    after: options.runOnAfter ? setFlag : undefined,
    onError: options.runOnError ? setFlag : undefined
  })
}

@dkatavic, what do you think?

[dkatavic]

Good catch @vladgolubev . I like the draft @lmammino , just I would like to make the middleware as configurable as possible, so can we add runOnBefore option too?

[lmammino]

That makes totally sense @dkatavic. Thanks!

Probably we should also set more efficient defaults like:

const defaults = {
    runOnBefore: true,
    runOnAfter: false,  // user code (or other middlewares) might reset the flag, but no extra function is executed
    runOnError: false  // user code (or other middlewares) might reset the flag, but no extra function is executed
  }

@vladgolubev, do you want to take care of submitting a PR with this? 😉

[vladholubiev]

@lmammino 🤔 🤝