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

serverless-offline raises warning due to callback usage #48

Open
ceilfors opened this issue Jun 1, 2019 · 7 comments

Comments

@ceilfors
Copy link
Member

@ceilfors ceilfors commented Jun 1, 2019

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

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Jun 21, 2019

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.

geoffdutton added a commit to geoffdutton/laconia that referenced this issue Jul 13, 2019
geoffdutton added a commit to geoffdutton/laconia that referenced this issue Aug 16, 2019
…o callback-warning

* 'callback-warning' of github.com:geoffdutton/laconia:
  removes callback pattern in core, laconiajs#48
  removes callback pattern in core
  npm audit fix
geoffdutton added a commit to geoffdutton/laconia that referenced this issue Aug 16, 2019
…o callback-warning

* 'callback-warning' of github.com:geoffdutton/laconia:
  add middleware test
  removes callback pattern in core
  removes callback pattern in core, laconiajs#48
  removes callback pattern in core
  npm audit fix
ceilfors added a commit that referenced this issue Aug 19, 2019
Remove callback pattern, part 2, #48
@ceilfors ceilfors closed this Aug 28, 2019
@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Aug 28, 2019

@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

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Aug 28, 2019

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

This comment has been minimized.

Copy link
Contributor

@geoffdutton geoffdutton commented Aug 28, 2019

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

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Aug 29, 2019

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.

ceilfors added a commit that referenced this issue Aug 31, 2019
ceilfors added a commit that referenced this issue Sep 3, 2019
Revert "Remove callback pattern, part 2, #48"
@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Sep 3, 2019

@geoffdutton I've reverted the change because I just realised that the use of .then works because if added a return statement. Upon removing the return statement, the test breaks which means other changes are necessary.

To help release our other changes, I'll revert this for now

@hugosenari

This comment has been minimized.

Copy link
Contributor

@hugosenari hugosenari commented Nov 11, 2019

we could at least ignore callback if it does not exist 🤔

hugosenari pushed a commit to hugosenari/laconia that referenced this issue Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.