Skip to content
This repository has been archived by the owner on Jun 18, 2020. It is now read-only.

[WIP] Refactor to support Promises and Async/Await #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

padiazg
Copy link

@padiazg padiazg commented Sep 17, 2019

Signed-off-by: Patricio Diaz [email protected]

This refactor is intended to give the node10-express template the ability to respond to promise or async/await based handler functions.

The handler in this PR has 3 examples for each scenario: callback, promise, async/await.

This proposal moves the control of sending the response to the middleware function in index.js.

When using the Promise and Async/Await flavors of handler, you must return the context variable with the resulting value you want to return.

If you are using the Callback flavor of handler, you must use the next function which is passed as the third parameter to handler to pass the control to middleware

How it was tested

For a Callback version of handler.js

$ faas new node10-express-cb --lang node10-express --prefix padiazg

handler.js

"use strict"

// callback version
const delayCallback = cb => {
    const min = 1; // 1 sec
    const max = 5; // 5 sec
    const delay = Math.round((Math.random() * (max - min) + min));
    setTimeout(() => cb(delay),  delay * 1000);
}

module.exports = (event, context, next) => {
    delayCallback(delay => {
        const result = {
            status: "You said: " + JSON.stringify(event.body),
            delay
        };
        
        context
          .status(200)
          .succeed(result);

        next();        
    })
}
$ faas up -f node10-express-cb.yml 
$ echo -n "async" | faas invoke node10-express-cb -
{"status":"You said: \"async\"","delay":2}

For a Promise version of handler.js

$ faas new node10-express-promise --lang node10-express --prefix padiazg

handler.js

"use strict"

// callback version
const delayCallback = cb => {
    const min = 1; // 1 sec
    const max = 5; // 5 sec
    const delay = Math.round((Math.random() * (max - min) + min));
    setTimeout(() => cb(delay),  delay * 1000);
}

// Uncomment the following line to use it in the promise or async/await versions
const delayPromise = () => new Promise((resolve, reject) => delayCallback(delay => resolve(delay)) )

// Promise version
module.exports = (event, context) => new Promise((resolve, reject) => {
    delayPromise()
    .then(delay => {
        const result =             {
            status: "You said: " + JSON.stringify(event.body),
            delay
        };
    
        context
            .status(200)
            .succeed(result);
            
        return resolve(context);
    })
});
$ faas up -f node10-express-promise.yml
$ echo -n "promise" | faas node10-express-promise -
{"status":"You said: \"promise\"","delay":2}

For a Async/Await version of handler.js

$ faas new node10-express-async --lang node10-express --prefix padiazg

handler.js

"use strict"

// callback version
const delayCallback = cb => {
    const min = 1; // 1 sec
    const max = 5; // 5 sec
    const delay = Math.round((Math.random() * (max - min) + min));
    setTimeout(() => cb(delay),  delay * 1000);
}

// Uncomment the following line to use it in the promise or async/await versions
const delayPromise = () => new Promise((resolve, reject) => delayCallback(delay => resolve(delay)) )

// async/await version
module.exports = async (event, context) => {  
    const delay = await delayPromise();

    const result =             {
        status: "You said: " + JSON.stringify(event.body),
        delay
    };

    context
        .status(200)
        .succeed(result);

    return context;
}
$ faas up -f node10-express-async.yml
$ echo -n "async" | faas invoke node10-express-async -
{"status":"You said: \"async\"","delay":2}

Signed-off-by: Patricio Diaz <[email protected]>
@padiazg padiazg changed the title Refactor to support Promises and Async/Await (WIP) [WIP] Refactor to support Promises and Async/Await Sep 18, 2019
@alexellis
Copy link
Member

Thank you for this PR 👍

module.exports = (event, context) => {
let err;
// callback version
const delayCallback = cb => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now part of the main template, was that intended or was it a mistake?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see handler.js as an example on how to build the code for the serverless function. As the main motivation for this refactoring is to handle asynchronicity, I use that function to simulate some load, and show how to take advantage of this new feature. But it's just a proposal, you can keep it or use the original example.

setTimeout(() => cb(delay), delay * 1000);
}

module.exports = (event, context, next) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next parameter is new I think? Why is it needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we use express we are using a framework, which is in control of the execution of our code, not us.
Having said that, the middleware function at index.js is in the "middle" of the execution flow, so we need to "fit" in that schema, node.js coders are used to and aware of that.
I think it's better if we just "build" the response in handler.js and let middleware in index.js take care of sending the response and handling errors.
Then once you build the response you call next to return control to the framework.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next parameter is supposed to be used when you return a callback to the middleware, it's not used when returning a Promise (Promise or Async/Await)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't line the next() parameter, it's confusing. mixes express and faas programming.
With that said, I believe callbacks should removed altogether, so it doesn't become an issue anymore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the use of the next() function it is confusing, but is something a express user is used to. I'm open to suggestions on how to solve the callback call in a different way. I already suggested that we need to drop support for it, but until that happens there must be a way to use it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way is to pass the res object to the handler function, with passes the control of the flow to it. I think the function main code has to be in control of that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or not use callbacks ;-)

I rebuilt a brand new template - thanks for your inspiration. will publish soon - I am testing right now. It also works differently, more inlined with lambda.

  • New better intuitive handler.js
  • Super tiny docker image using multistage build, down to 65MB (from 118MB!)
  • Garbage collector manually triggered every 30s
  • Image properly handles signals using a graceful shutdown
  • Catch all uncaught exceptions and unhandled promise rejections

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's something I already asked about, what happens if we don't use callbacks at all

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about how the handler.js template changed.

@padiazg
Copy link
Author

padiazg commented Oct 14, 2019

As a personal comment, I think we can start thinking on dropping support for Callbacks and just support Promises, I think nobody would complain nowadays.

@orefalo
Copy link

orefalo commented Oct 29, 2019

generally speaking I don't like the way openfaas handles the context. context and results should be two different structures.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants