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

Unable to run the lambda asynchronously #78

Closed
SuchismitaGoswami opened this issue Aug 29, 2021 · 21 comments · Fixed by #89
Closed

Unable to run the lambda asynchronously #78

SuchismitaGoswami opened this issue Aug 29, 2021 · 21 comments · Fixed by #89
Labels

Comments

@SuchismitaGoswami
Copy link

SuchismitaGoswami commented Aug 29, 2021

I am having the app.js code like below. While invoking the lambda asynchronously, it's not waiting for any event emitter callback to execute.

module.exports = async (app) => {
  // Your code here
  app.log.info("Yay, the app was loaded!");

  await new Promise(resolve => {
    app.log.info('waiting for event....')
    app.on("issues.opened", async (context) => {
      const issueComment = context.issue({
        body: "Thanks for opening this issue!",
      });
      resolve(context.octokit.issues.createComment(issueComment));
    });

    app.on("deployment_status.created", async (context) => {

      let deployment_status = context.payload.deployment_status.state
      app.log.info(`deployment_status created: ${deployment_status}`)
      const config = await context.config('config.yaml');

      if (deployment_status == 'success') {
        app.log.info(context.repo().repo)
        app.log.info(`Deployment - exclude environement ${config.deployment.environment.excludes}`)
        app.log.info(sign(context.payload, "development"))
        resolve()
      }
    });

  })
  app.log.info('Exiting ....')
};
@SuchismitaGoswami
Copy link
Author

Please help me to resolve this issue. I confirm that while invoking the lambda running the GitHub app code, it works as expected

@gr2m
Copy link
Contributor

gr2m commented Aug 29, 2021

the probot function code has to resolve before any webhooks can be received, but with your implementation it can only resolve after it received the first event, so you are in a deadlock, it won't work like this.

Try this instead

module.exports = async (app) => {
  // Your code here
  app.log.info("Yay, the app was loaded!");

  app.log.info("waiting for event....");
  app.on("issues.opened", async (context) => {
    const issueComment = context.issue({
      body: "Thanks for opening this issue!",
    });
    return context.octokit.issues.createComment(issueComment);
  });

  app.on("deployment_status.created", async (context) => {
    let deployment_status = context.payload.deployment_status.state;
    app.log.info(`deployment_status created: ${deployment_status}`);
    const config = await context.config("config.yaml");

    if (deployment_status == "success") {
      app.log.info(context.repo().repo);
      app.log.info(
        `Deployment - exclude environement ${config.deployment.environment.excludes}`
      );
      app.log.info(sign(context.payload, "development"));
    }
  });
  app.log.info("Exiting ....");
};

@SuchismitaGoswami
Copy link
Author

@gr2m Thank you so much for your prompt response. I tried the above code as you mentioned. Unfortunately, while invoking the lambda asynchronously, it's not waiting for any webhook request. Below is my lambda handler written using the aws-lambda-serverless adapter

const {
    createLambdaFunction,
    createProbot,
} = require("@probot/adapter-aws-lambda-serverless");
const appFn = require("./app");

module.exports.webhooks = createLambdaFunction(appFn, {
    probot: createProbot(),
});

@gr2m
Copy link
Contributor

gr2m commented Aug 30, 2021

Unfortunately, while invoking the lambda asynchronously, it's not waiting for any webhook request

That's as it should be, it's all good.

You can remove this line

app.log.info("Exiting ....");

It gets executed before any of the webhooks handlers is.

@SuchismitaGoswami
Copy link
Author

SuchismitaGoswami commented Aug 30, 2021

The issue is the lambda function is not waiting until the event loop is empty. So, the probot app is not executing any of the webhook handler registered. How to resolve that issue?

Important Note: I am invoking the lambda asynchronously from the API Gateway

This is how the log looks like
image
No webhook handler is being executed. Could you please help me fix this issue?

@gr2m
Copy link
Contributor

gr2m commented Aug 30, 2021

I'm not a lambda expert, I'm out of my depth here I'm afraid. We have an example app that is using this adapter and it worked:
https://github.com/probot/example-aws-lambda-serverless/

Maybe there is some clue in there that might help?

I hope you can figure it out, please let us know when you do

GitHub
Probot & AWS Lamda example using Serverless. Contribute to probot/example-aws-lambda-serverless development by creating an account on GitHub.

@SuchismitaGoswami
Copy link
Author

SuchismitaGoswami commented Aug 30, 2021

Thank you so much for your response. I am a little confused why the lambda function is not waiting for the probot app to load. https://github.com/probot/adapter-aws-lambda-serverless/blob/master/index.js#L14 . Could you please let me know is there anything I am missing out here?

GitHub
An extension for running Probot on Lambda. Contribute to probot/adapter-aws-lambda-serverless development by creating an account on GitHub.

@SuchismitaGoswami
Copy link
Author

I figured out that the issue was with this line

context.callbackWaitsForEmptyEventLoop = false;

By default, this value is true and callback will wait until the Node.js runtime event loop is empty before freezing the process and returning the results to the caller. By setting it to false, it's request AWS Lambda to freeze the process soon after the callback is called, even if there are events in the event loop. AWS Lambda will freeze the process, any state data and the events in the Node.js event loop.

@gr2m
Copy link
Contributor

gr2m commented Aug 30, 2021

I figured out that the issue was with this line

context.callbackWaitsForEmptyEventLoop = false;

That's what we are setting here right?

// Ends function immediately after callback
context.callbackWaitsForEmptyEventLoop = false;

You are saying that setting it to false is causing problems for you? Shall we remove the line? I'm not exactly sure what the implications are, maybe we need to make it conditional?

@SuchismitaGoswami
Copy link
Author

SuchismitaGoswami commented Aug 30, 2021

I am also not 100% sure about its implication. But, after changing the lambda handler to the below, it worked as expected.
For testing purpose, instead of using createLambdaFunction API, I copied the same code to my handler.

const {
    createProbot
} = require("@probot/adapter-aws-lambda-serverless");
const appFn = require("./app");
const lowercaseKeys = require("lowercase-keys");

let probot = createProbot()
probot.load(appFn);


module.exports.webhooks = async (event, context) => {
    try {
        context.callbackWaitsForEmptyEventLoop = true
        const headersLowerCase = lowercaseKeys(event.headers);

        await probot.webhooks.verifyAndReceive({
            id: headersLowerCase["x-github-delivery"],
            name: headersLowerCase["x-github-event"],
            signature:
                headersLowerCase["x-hub-signature-256"] ||
                headersLowerCase["x-hub-signature"],
            payload: event.body
        });
        return {
            statusCode: 200,
            body: '{"ok":true}',
        };
    } catch (error) {
        return {
            statusCode: error.status || 500,
            error: "ooops",
        };
    }

}

So, maybe making it conditional would be the best.

@dubsalot
Copy link

dubsalot commented Dec 5, 2021

I had a similar issue, and this is how I resolved:

module.exports.webhooks = createLambdaFunction(setUpFunction, {
    probot: createProbot()
})



// your setup function should be like so - This is setUpFunction()
module.exports = (pbapp) => {
    pbapp.log.info("Initializing Lambda");
    pbapp.onError((error) => {
        pbapp.log.error(error);
    });
    let lib = require("@lib/handle_some_event");    // I keep my code organized, or try to.
    pbapp.on('some_event', lib.handle_some_event);   //for exammple, deployment_status.created is an event
    return pbapp;
};


// @lib/handle_some_event
// 
exports.handle_some_event = (context) => {
  context.log.info("handling some event");
  
  //notice we are returning a promise
  return new Promise((resolve, reject) => {
        //DoSomeLongAsyncCallHere  - note that this method also returns a promise.
        //For example, this could be a function that drops a file on S3.
        DoSomeLongAsyncCallHere().then((response) => {resolve(response)}).catch( (error) => {reject(error)});
  });
};

@ajschmidt8
Copy link
Contributor

ajschmidt8 commented Mar 8, 2022

I'm experiencing this issue as well. I've discovered a few things that I figured I would add to this conversation.

First, here are some official AWS docs that talk about the purpose of callbackWaitsForEmptyEventLoop:

It seems like that option is only relevant to non-async functions (i.e. functions that use callbacks). Since the lambdaFunction function is async, I think it would be safe to remove that line entirely (which I presume would set the value back to true).

It seems like this issue only appears when using async: true with the http event (async: true is not supported with httpApi events). Here's a summary of my working/non-working serverless.yaml configuration excerpts:

# this doesn't work
functions:
  webhooks:
    handler: handler.webhooks
    events:
      - http:
          path: /api/github/webhooks
          method: post
          async: true

# this works
functions:
  webhooks:
    handler: handler.webhooks
    events:
      - http:
          path: /api/github/webhooks
          method: post
#          async: true

# this works
functions:
  webhooks:
    handler: handler.webhooks
    events:
      - httpApi:
          path: /api/github/webhooks
          method: post
#          async: true # this is commented out because the `httpApi` event does not support asynchronous invocations

After going through the git history for the repo, it seems that the function in question was initially committed as a non-async function that used callbacks, but was later converted to an async function without removing the context.callbackWaitsForEmptyEventLoop = false call:

So based on everything outlined above, I think it should be safe to remove context.callbackWaitsForEmptyEventLoop = false entirely and I will open a PR to do so 🙂.

When Lambda runs your function, it passes a context object to the handler . This object provides methods and properties that provide information about the invocation, function, and execution environment.
The following example function logs the contents of the event object and returns the location of the logs.

@gr2m
Copy link
Contributor

gr2m commented Mar 8, 2022

So based on everything outlined above, I think it should be safe to remove context.callbackWaitsForEmptyEventLoop = false entirely and I will open a PR to do so 🙂.

great research, thank you! I'm not using the adapter myself, so if you could send the PR I'm happy to merge, I'd just ask you to confirm that things are working as expected with the new version. If you could update any relevant docs as part of the PR that would be great

@ajschmidt8
Copy link
Contributor

great research, thank you! I'm not using the adapter myself, so if you could send the PR I'm happy to merge, I'd just ask you to confirm that things are working as expected with the new version. If you could update any relevant docs as part of the PR that would be great

No problem! I'm testing locally now. I will open a PR assuming everything works as expected.

@ajschmidt8
Copy link
Contributor

🙁 that actually didn't resolve the issue. I will continue investigating. I think it can still be removed regardless.

@ajschmidt8
Copy link
Contributor

Hmm. I can confirm that @SuchismitaGoswami's inlined excerpt above works correctly though. Here's mine, adapted for TypeScript.

import { createProbot } from "@probot/adapter-aws-lambda-serverless";
import appFn from "./index";
import lowercaseKeys from "lowercase-keys";

let probot = createProbot();
probot.load(appFn);

export const webhooks = async (event, context) => {
  try {
    const headersLowerCase: any = lowercaseKeys(event.headers);

    await probot.webhooks.verifyAndReceive({
      id: headersLowerCase["x-github-delivery"],
      name: headersLowerCase["x-github-event"],
      signature:
        headersLowerCase["x-hub-signature-256"] ||
        headersLowerCase["x-hub-signature"],
      payload: event.body,
    });
    return {
      statusCode: 200,
      body: '{"ok":true}',
    };
  } catch (error: any) {
    return {
      statusCode: error.status || 500,
      error: "ooops",
    };
  }
};

I think this could imply that there's something wrong with index.js? Continuing my investigation.

@ajschmidt8
Copy link
Contributor

I'm stumped for now, but here's what I've found.

With this async: true config mentioned above,

functions:
  handleProbot:
    handler: dist/serverless.webhooks
    events:
      - http:
          path: /api/github/webhooks
          method: POST
          async: true

I can inline some of the adapter-aws-lambda-serverless functions like this and it works as expected:

import { createProbot } from "@probot/adapter-aws-lambda-serverless";
import appFn from "./index";
import lowercaseKeys from "lowercase-keys";

const lambdaFunction = async (probot, event, context) => {
  try {
    context.callbackWaitsForEmptyEventLoop = false;
    const headersLowerCase: any = lowercaseKeys(event.headers);

    await probot.webhooks.verifyAndReceive({
      id: headersLowerCase["x-github-delivery"],
      name: headersLowerCase["x-github-event"],
      signature:
        headersLowerCase["x-hub-signature-256"] ||
        headersLowerCase["x-hub-signature"],
      payload: event.body,
    });
    return {
      statusCode: 200,
      body: '{"ok":true}',
    };
  } catch (error: any) {
    return {
      statusCode: error.status || 500,
      error: "ooops",
    };
  }
};

export const webhooks = (() => {
  let probot = createProbot();
  probot.load(appFn);

  return lambdaFunction.bind(null, probot);
})();

But if I use the package functions from the node_modules folder like this, it doesn't work:

import {
  createLambdaFunction,
  createProbot,
} from "@probot/adapter-aws-lambda-serverless";
import app from "./index";

export const webhooks = createLambdaFunction(app, {
  probot: createProbot(),
});

Not really sure what to make of this, but I will pick this back up tomorrow. I really need the async: true to work.

@ajschmidt8
Copy link
Contributor

I am also not 100% sure about its implication. But, after changing the lambda handler to the below, it worked as expected. For testing purpose, instead of using createLambdaFunction API, I copied the same code to my handler.

const {
    createProbot
} = require("@probot/adapter-aws-lambda-serverless");
const appFn = require("./app");
const lowercaseKeys = require("lowercase-keys");

let probot = createProbot()
probot.load(appFn);


module.exports.webhooks = async (event, context) => {
    try {
        context.callbackWaitsForEmptyEventLoop = true
        const headersLowerCase = lowercaseKeys(event.headers);

        await probot.webhooks.verifyAndReceive({
            id: headersLowerCase["x-github-delivery"],
            name: headersLowerCase["x-github-event"],
            signature:
                headersLowerCase["x-hub-signature-256"] ||
                headersLowerCase["x-hub-signature"],
            payload: event.body
        });
        return {
            statusCode: 200,
            body: '{"ok":true}',
        };
    } catch (error) {
        return {
            statusCode: error.status || 500,
            error: "ooops",
        };
    }

}

So, maybe making it conditional would be the best.

@SuchismitaGoswami, can you try re-running this code block with context.callbackWaitsForEmptyEventLoop = false and confirm that it still works? That's the behavior that I'm seeing and I'm having trouble making sense of this 🤔. Was hoping you could provide a sanity check.

ajschmidt8 added a commit to ajschmidt8/adapter-aws-lambda-serverless that referenced this issue Mar 9, 2022
Fixes probot#78.

This PR removes the `JSON.parse` call from `event.body`. This was causing issues when trying to invoke Lambda functions asynchronously with `async` set to `true` in a `serverless.yaml` config file. I'm not exactly sure why this doesn't cause issues when `async` is not set to `true`, but I tested these changes with `http` events (both `async: true` and `async: false`) and `httpApi` events and it seemed to work fine on both. Also, I believe the `verifyAndReceive` function internally handles both objects and strings anyway.
ajschmidt8 added a commit to ajschmidt8/adapter-aws-lambda-serverless that referenced this issue Mar 9, 2022
`callbackWaitsForEmptyEventLoop` is only needed for non-async functions (i.e. functions that use callbacks) and therefore can be safely removed from this package.

See probot#78 (comment) for more details
@ajschmidt8
Copy link
Contributor

ajschmidt8 commented Mar 9, 2022

Fixed it! This one was a challenge since errors aren't propagated when async is set to true in serverless.yaml. Ultimately it seems that the JSON.parse call in #89 was the culprit. After removing that, everything worked as expected. From re-inspecting the inlined code above, it seems that the JSON.parse call was removed by @SuchismitaGoswami in her code and that was likely why it worked (not the callbackWaitsForEmptyEventLoop = true change).

#90 and #91 also came out of these debugging efforts.

@SuchismitaGoswami
Copy link
Author

SuchismitaGoswami commented Mar 9, 2022

@ajschmidt8 Sure, I will give it a try and do the sanity check. I am a little busy this week due to personal reason. I will get back to this thread with my findings. But, great work on finding out the reason behind it. TBH, I hardly thought the issue could be due to JSON.parse when I raised this. I intentionally removed it from my code as, in my case, the event was already a JSON object. So, great research.

@gr2m gr2m closed this as completed in #89 Mar 9, 2022
gr2m pushed a commit that referenced this issue Mar 9, 2022
`callbackWaitsForEmptyEventLoop` is only needed for non-async functions (i.e. functions that use callbacks) and therefore can be safely removed from this package.

See #78 (comment) for more details
@github-actions
Copy link

github-actions bot commented Mar 9, 2022

🎉 This issue has been resolved in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

4 participants