Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interrupting middlewares running progress #72

Closed
NPCRUS opened this issue Jan 8, 2018 · 10 comments
Closed

Interrupting middlewares running progress #72

NPCRUS opened this issue Jan 8, 2018 · 10 comments

Comments

@NPCRUS
Copy link
Contributor

NPCRUS commented Jan 8, 2018

Hello guys. Is there a way to interrupt middleware in the middle of execution and execute callback to return something as result as in example

module.exports = (config) => {
    return ({
        before: (handler, next) => {
            if(true) {
                console.log('Lambda is warm!')
                return handler.callback(null, 'Lambda is warm!')
            }

            next()
        }
    })
  }

Or is there any other way to do it?

Best regards

@lmammino
Copy link
Member

That's definitely a fair point.

We should definitely add an example like this in the doc and a test.

Plus it might be interesting to create a "keep lambda warm" middleware in the default middleware package

@NPCRUS
Copy link
Contributor Author

NPCRUS commented Jan 10, 2018

I investigated this.
Actually, code as in example works fine

module.exports = (config) => {
    return ({
      before: (handler, next) => {
        if(handler.event.warmup === true) {
            console.log('warmup - done')
            return handler.context.succeed()
        }

        next()
      }
    })
  }

instead of succeed method we can use done and fail

I can make a PR with some additions to the docs and middleware code

@lmammino
Copy link
Member

Great example @NPCRUS, thanks.

The only thing I would be concerned is that when you run this you are going to skip any middleware in any other phase (after/error), so there might be unexpected behaviours.

Let's just imagine the case that you have an output serializer and an after middleware, would you assume that an early call of the callback should anyway use the serializer or not?

Plus it would be great to have an example like this documented, tested and maybe with a simplified API (rather than having to resort to the old context.succeed.

So, quite open for discussion here, feel free to provide more inputs and ideas.

Calling also @DavidWells and @dkatavic for discussing this :)

@dkatavic
Copy link
Contributor

Great point @NPCRUS

As @lmammino stated, I would like to figure out what would be the best behaviour after middlewares. I will give it some thought and report a bit later. Thanks for the contribution @NPCRUS :)

@NPCRUS
Copy link
Contributor Author

NPCRUS commented Jan 10, 2018

@lmammino The point of warming up lambdas is to skip any logic and other things and just make the container running.
So, if you are using serverless-plugin-warmup, it just executes you lambda from other lambda function and you don't need to execute anything except this middleware. in this case it's a good point to exit from warmup-check middleware.

Other thing here is maybe to provide some sort of check function here as in example:

middy(handler)
  .use(warmUpCheckMiddleware(handler => handler.event.source === 'serverless-warmup-plugin'))

So, not everyone uses serverless-warmup-plugin, and condition in example may change

@lmammino
Copy link
Member

Great idea with the warmUpCheckMiddleware, I would move that to a separate issue and draft a specification for it, so yourself or somebody else can pick it up for implementation.

In this issue, I will keep focusing more on how we make easy to write middlewares that can stop the execution flow

@DavidWells
Copy link
Contributor

I think this is great example of how to abort if a condition is met

Let's just imagine the case that you have an output serializer and an after middleware, would you assume that an early call of the callback should anyway use the serializer or not?

I think if handler.context.succeed() doesn't short circuit the function execution it could lead to some unintended consequences.

Like I could have output middleware logging stuff etc that I only want to run when the logic of the function runs and not on ping events. If I have to also update middleware down the chain with if/else's to also bail (or next()) if event.warmup === true. It could get messy quick.

If you handler.context.succeed() or handler.context.fail(), the function ends execution then and there.

One thing I'd add as a "best practice" is a clearer log message to that example:

module.exports = (config) => {
    return ({
      before: (handler, next) => {
        if (handler.event.warmup === true) {
            console.log('👋 Exiting early via warmUpMiddleware')
            return handler.context.succeed()
        }

        next()
      }
    })
  }

How does express/koa handle early middleware bails?

This was referenced Jan 11, 2018
@NPCRUS
Copy link
Contributor Author

NPCRUS commented Jan 11, 2018

Another one thing here is that context.succeed() thing is not clear for everyone. It was used in previous lambda versions, now official way to return some result from lambda is callback(err, result).
So maybe we should pass callback in handler object, so we can use it middlewares like in example:
handler.callback()
And supply it with some documentation, so everyone will be available to exit from his middlewares withput searching on stackoverflow for context.succeed method

@NPCRUS
Copy link
Contributor Author

NPCRUS commented Jan 11, 2018

@DavidWells Express/koa provide res/req object in any middleware in chain. So in any middleware you can execute return res.end() and don't call next() and it will cause quiting from middleware chain.

@lmammino
Copy link
Member

@NPCRUS there's a PR open at #77, feel more than welcome to check it out and give your feedback on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants