serverless-offline raises warning due to callback usage

[ceilfors]

Describe the bug
This bug is originally raised by @Strernd on gitter. When Laconia is used together with serverless-offline, it is raising a warning.

To Reproduce
Use Laconia with serverless-offline

Expected behavior
A clear and concise description of what you expected to happen.

Actual behavior
A warning message generated by serverless-offline:

Serverless: Warning: handler 'laconia-handler' returned a promise and also uses a callback! | This is problematic and might cause issues in your lambda.

Additional context
The callback is originally retained even though we support node 8 because of a problem in AWS Lambda. Seems like this problem is now fixed, see: https://stackoverflow.com/a/51988695/2464295

[ceilfors]

Removing the use of callback will improve the ease of catching errors in middlewares too, as at the moment users who are trying to catch errors in middleware will have to implement a callback.

[ceilfors]

@geoffdutton Upon running the acceptance-test in laconia, this change of code apparently is breaking @laconia/batch behaviour. The break has nothing to do with our code, but AWS Lambda itself. Without the use of callback, AWS Lambda doesn't seem to be waiting for the event loop to be empty before ending its execution, hence the self invocation mechanism (recursion) that's supposed to happen in @laconia/batch is not happening.

This problem is similar to the behaviour that has been described here: https://forums.aws.amazon.com/thread.jspa?messageID=871017

[ceilfors]

Unfortunately the advice from that forum doesn't seem to work. I have attempted to remove the async from top level, by changing the code to look like this

  const laconia = (event, context) => {
    laconiaContext.registerInstances({ event, context });

    return laconiaContext.refresh().then(() => app(event, laconiaContext));
  };

And it doesn't seem to still be waiting for the event loop to be empty.

Reverting the code has definitely make the acceptance-test to pass. Any other alternatives before we attempt to revert the code?

[geoffdutton]

I was trying to run the acceptance tests as well and couldn't get them to work. This makes sense. What if we make the main laconia function something like:

// This would be the raw Lambda event, context, and callback
const laconia = (event, context, callback) => {
    laconiaContext.registerInstances({ event, context });

    laconiaContext.refresh()
        .then(() => app(event, laconiaContext));
        .then(res => {
            callback(null, res)
        })
        .catch(err => {
            callback(err)
        })
};

I think if we don't return a promise and only use the call back at the very top level, maybe it works? I'll try to test it tonight or tomorrow.

Edit: There is the context.callbackWaitsForEmptyEventLoop, which I've had to set to false before because a MySQL database connection was causing the Lambda to timeout, but this sounds like the opposite issue...

[ceilfors]

On running the acceptance test, do leave your issues or feedback in #104 or #99.

Yes, this is the opposite problem of callbackWaitsForEmptyEventLoop, and I presume it's a bug.. The suggestion you made works as the acceptance test is passing! Just had to add a return statement.

Even though it works, I'm quite torn apart with the use of callback. One of the big win in term of design when callback is not used is, when a middleware is being written, they will not have to handle any callback at all. They will be able to handle error flow just by using try catch.

I had a use case where I wanted to have a highest level middleware where I handle errors differently, but unfortunately I had to pass callback all the way down, and check if the callback is called with an error object. The removal of callback will make this use case super easy.

I wonder if there's a way to call the callback on the very last minute or something, maybe by calling setImmediate. I'll experiment tonight.