probot / adapter-aws-lambda-serverless

An extension for running Probot on Lambda
ISC License
94 stars 36 forks source link

Unable to run the lambda asynchronously #78

Closed SuchismitaGoswami closed 2 years ago

SuchismitaGoswami commented 3 years ago

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 commented 3 years ago

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

gr2m commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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
GitHub - probot/example-aws-lambda-serverless: Probot & AWS Lamda example using Serverless
Probot & AWS Lamda example using Serverless. Contribute to probot/example-aws-lambda-serverless development by creating an account on GitHub.
SuchismitaGoswami commented 3 years ago

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
adapter-aws-lambda-serverless/index.js at master · probot/adapter-aws-lambda-serverless
An extension for running Probot on Lambda. Contribute to probot/adapter-aws-lambda-serverless development by creating an account on GitHub.
SuchismitaGoswami commented 3 years ago

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 commented 3 years ago

I figured out that the issue was with this line

context.callbackWaitsForEmptyEventLoop = false;

That's what we are setting here right?

https://github.com/probot/adapter-aws-lambda-serverless/blob/2b6a1f48d954bfe4b78d8c648c46e69b4326b7e1/lambda-function.js#L7-L8

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :slightly_smiling_face:.

AWS Lambda context object in Node.js - AWS Lambda
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.
AWS Lambda function handler in Node.js - AWS Lambda
The following example function logs the contents of the event object and returns the location of the logs.
gr2m commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ajschmidt8 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :thinking:. Was hoping you could provide a sanity check.

ajschmidt8 commented 2 years ago

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 commented 2 years ago

@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.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 3.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: