-
Notifications
You must be signed in to change notification settings - Fork 393
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
processBeforeRepsonse: true doesn't work when a Bolt listener function has WebClient calls inside #462
Comments
I have tried reproducing this issue locally but the behavior works as expected so far. First, I added a few log statements to diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts
index f125bfa..0ea0d3c 100644
--- a/src/ExpressReceiver.ts
+++ b/src/ExpressReceiver.ts
@@ -72,6 +72,7 @@ export default class ExpressReceiver implements Receiver {
const event: ReceiverEvent = {
body: req.body,
ack: async (response): Promise<void> => {
+ this.logger.debug('ack() begin');
if (isAcknowledged) {
throw new ReceiverMultipleAckError();
}
@@ -82,6 +83,7 @@ export default class ExpressReceiver implements Receiver {
} else {
storedResponse = response;
}
+ this.logger.debug('ack() response stored');
} else {
if (!response) {
res.send('');
@@ -90,18 +92,21 @@ export default class ExpressReceiver implements Receiver {
} else {
res.json(response);
}
+ this.logger.debug('ack() response sent');
}
},
};
try {
await this.bolt?.processEvent(event);
+ this.logger.debug('event processed');
if (storedResponse !== undefined) {
if (typeof storedResponse === 'string') {
res.send(storedResponse);
} else {
res.json(storedResponse);
}
+ this.logger.debug('stored response sent');
}
} catch (err) {
res.send(500); Next, I ran the following program twice. Once with
Here are the outputs:
|
I'm going to keep trying to reproduce this issue. My next attempt will be to put the code that makes the requests to the Bolt app in a separate process from the Bolt app itself. I think it's interesting that the |
@seratch i'm going to remove the i used |
also, v2.0 is released so i think i should close that milestone too. |
My intention of putting v2.0 milestone is marking the changes that will be included 2.0 patch releases but it may be a bit irregular way of the milestone's usage. I agree that we can close it as we've already released the initial version of the series. |
@aoberoi Here is a reproducer.
I can put the Update: I've figured out index.jsconst { LogLevel } = require("@slack/logger");
const logLevel = process.env.SLACK_LOG_LEVEL || LogLevel.INFO;
const { App } = require("@slack/bolt");
const app = new App({
logLevel: logLevel,
signingSecret: process.env.SLACK_SIGNING_SECRET,
token: process.env.SLACK_BOT_TOKEN,
processBeforeResponse: true,
});
app.use(async (args) => {
args.logger.info("*** middleware ***");
const result = await args.next();
args.logger.info("after next() in a middleware");
return result;
});
app.event("app_mention", async ({ logger, event, say }) => {
logger.info("*** app_mention event ***");
const result = await say({ text: `:wave: <@${event.user}> Hi there!` });
logger.info("after say");
return result;
});
app.command("/open-modal", async ({ logger, client, ack, body }) => {
logger.info("*** slash command ***");
try {
const res = await client.views.open({
"trigger_id": body.trigger_id,
"view": {
"type": "modal",
"callback_id": "task-modal",
"title": {
"type": "plain_text",
"text": "Create a task"
},
"submit": {
"type": "plain_text",
"text": "Submit"
},
"close": {
"type": "plain_text",
"text": "Cancel"
},
"blocks": [
{
"type": "input",
"block_id": "input-title",
"element": {
"type": "plain_text_input",
"action_id": "input"
},
"label": {
"type": "plain_text",
"text": "Title"
},
"optional": false
}
]
}
});
logger.info("after views.open");
await ack();
logger.info("after ack");
} catch (e) {
logger.error("views.open error:\n\n" + JSON.stringify(e, null, 2) + "\n");
await ack(`:x: Failed to open a modal due to *${e.code}* ...`);
}
});
app.view("task-modal", async ({ logger, client, body, ack }) => {
logger.info("*** view submission ***");
await ack();
logger.info("after ack");
});
(async () => {
await app.start(process.env.PORT || 3000);
console.log("⚡️ Bolt app is running!");
})(); Here is another version with promise chain style. app.use(async (args) => {
args.logger.info("*** middleware ***");
return args.next().then(result => {
args.logger.info("after next() in a middleware");
return result;
})
});
app.event("app_mention", async ({ logger, event, say }) => {
logger.info("*** app_mention event ***");
return say({ text: `:wave: <@${event.user}> Hi there!` })
.then(() => logger.info("after say"));
});
app.command("/open-modal", async ({ logger, client, ack, body }) => {
logger.info("*** slash command ***");
return client.views.open({
"trigger_id": body.trigger_id,
"view": {
"type": "modal",
"callback_id": "task-modal",
"title": {
"type": "plain_text",
"text": "Create a task"
},
"submit": {
"type": "plain_text",
"text": "Submit"
},
"close": {
"type": "plain_text",
"text": "Cancel"
},
"blocks": [
{
"type": "input",
"block_id": "input-title",
"element": {
"type": "plain_text_input",
"action_id": "input"
},
"label": {
"type": "plain_text",
"text": "Title"
},
"optional": false
}
]
}
})
.then(() => logger.info("after views.open"))
.then(() => ack())
.then(() => logger.info("after ack"))
.catch((e) => {
logger.error("views.open error:\n\n" + JSON.stringify(e, null, 2) + "\n");
return ack(`:x: Failed to open a modal due to *${e.code}* ...`);
});
});
app.view("task-modal", async ({ logger, client, body, ack }) => {
logger.info("*** view submission ***");
return ack().then(() => logger.info("after ack"));
}); package.json{
"name": "bolt-starter",
"version": "0.0.1",
"description": "Bolt app starter",
"main": "index.js",
"scripts": {
"local": "node_modules/.bin/nodemon index.js",
"start": "node index.js"
},
"repository": {
"type": "git",
"url": "[email protected]:seratch/bolt-starter.git"
},
"keywords": [
"Slack",
"Bolt"
],
"author": "@seratch",
"license": "MIT",
"bugs": {
"url": "https://github.com/seratch/bolt-starter/issues"
},
"homepage": "https://github.com/seratch/bolt-starter#readme",
"dependencies": {
"@slack/bolt": "^2.0.0",
"dotenv": "^8.2.0"
},
"devDependencies": {
"nodemon": "^2.0.1"
}
} node_modules/@slack/bolt/dist/ExpressReceiver.jsModified await ((_a = this.bolt) === null || _a === void 0 ? void 0 : _a.processEvent(event));
console.log(`processBeforeResponse: ${this.processBeforeResponse}, stored response: ${storedResponse}`); // ADDED
if (storedResponse !== undefined) {
if (typeof storedResponse === 'string') {
res.send(storedResponse);
}
else {
res.json(storedResponse);
}
console.log(`stored response sent`); // ADDED
} outputs
|
As I shared above, the main issue we're still tackling is that HTTP requests by axios always run separately from an async listener/middleware function. I'm still not sure if there is a way to take control from the Bolt framework side (even not sure if it's specific to axios or it can happen with other 3rd parties yet). In my understanding, there are three options here:
@aoberoi @stevengill @shaydewael I prefer 0️⃣ > 2️⃣ > 1️⃣ but I can agree with any decision. For 0️⃣, I don't have the bandwidth to take it this week, so that I would like someone else (any contributions from the community are of course welcome!) to do a deep dive. |
Thanks for making a reproducing case. I've run it myself and verified. I also agree with the priority of these solutions. However, I think the only way to achieve 2️⃣ without a breaking API change is through an additional option on |
To be clear, for 2️⃣, I think we can add a new option with a default value that is backward compatible (if I don't miss something). Let's say the option is |
I agree that we can add an option and it can remain backwards compatible. The problem is that example code such as the following won't work for anyone who sets the option to app.event('app_mention', async ({ event, say }) => {
await say(`:wave: <@${event.user}> Hi there!`);
}); Likewise, sample code like the following won't work for anyone who sets the option to app.event('app_mention', async ({ event, ack, say }) => {
await ack();
await say(`:wave: <@${event.user}> Hi there!`);
}); This results in fragmentation in our docs, sample code, and other places. This is feasible, but in my opinion makes this solution a much less attractive option. |
I've been trying to narrow down this situation and I'm seeing some very strange (non-deterministic) results. I'm running a pretty minimal sample program: const { App, LogLevel } = require('./dist');
const processBeforeResponse = true;
const signingSecret = process.env.SLACK_SIGNING_SECRET;
const app = new App({
signingSecret,
processBeforeResponse,
logLevel: LogLevel.DEBUG,
ignoreSelf: false,
authorize() { return Promise.resolve({ }); },
});
app.event('app_mention', async ({ event, say }) => {
console.log('*** app_mention event ***');
try {
await say(`:wave: <@${event.user}> Hi there!`);
} catch (error) {
// say will not work without a real token, so catch the error to allow the listener to resume
console.error(error);
}
console.log('after say');
});
app.event('app_mention_fake', async ({ event, say }) => {
console.log('*** app_mention_fake event ***');
await fakeSay();
console.log('after fakeSay');
});
app.event('app_mention_sync', async () => {
console.log('*** app_mention_sync event ***');
});
(async () => {
await app.start(8080);
console.log('⚡️ Bolt app is running!');
})();
function fakeSay() {
return new Promise((resolve) => {
// random delay between 1-6 seconds
setTimeout(resolve, (Math.random() * 5000) + 1000);
});
} I also have the In the first event listener ( If what we landed on earlier was true (
For both event listeners, The confusing part is these results directly conflict with the results of the test I did earlier. |
One more data point: when I remove the HTTP client code ( My next step will be to do some breakpoint debugging to understand the execution order a little better. |
I've figured it out! It was much simpler than I thought it would be. The In the following code, the return value of This led to a "fork" of the middleware chain. One side of the fork held all the following middleware processing. The other side resolved, allowing the Promise returned by I'm preparing a PR with a fix now. |
and as for why my test case above passed... it's annoyingly simple. my simulated Events API request and block action request didn't contain a conversation ID. so the |
Thanks for working on this and I'm happy to know you've figured out the cause! The above explanation is very clear and that makes sense a lot to me. I haven't verified if applying the fix on conversation store fixes the issue yet. If you need someone to double-check, I can do that. But if you're already confident enough, let's ship the fix as soon as possible. |
Description
When deploying a Bolt app to AWS Lambda, and using
processBeforeResponse
option, the listener is not able to complete before the HTTP response is finished.This issue was reported by a user in the Slack Community workspace: https://community.slack.com/archives/CHY642221/p1585670255001600.
Requirements (place an
x
in each of the[ ]
)Bug Report
Filling out the following details about bugs will help us solve your issue sooner.
Reproducible in:
package version: 2.0.0
node version: NA
OS version(s): NA
Steps to reproduce:
Use the following code:
Deploy to AWS Lambda
Open the App Home in Slack to trigger the event subscription.
Expected result:
User should receive the message sent using
say()
, and the logs should show "finish to say".Actual result:
There's no message and the line does not appear in the logs.
The text was updated successfully, but these errors were encountered: