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

Re-fix #395 by adjusting the processBeforeResponse option #459

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions src/App.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,89 @@ describe('App', () => {
assert.equal(fakeErrorHandler.callCount, dummyReceiverEvents.length);
});
});

describe('processBeforeResponse', () => {

it('should call ack() after event listener completion', async () => {
// Arrange
const App = await importApp(overrides); // tslint:disable-line:variable-name
const fakeLogger = createFakeLogger();
const app = new App({
logger: fakeLogger,
receiver: fakeReceiver,
authorize: sinon.fake.resolves(dummyAuthorizationResult),
processBeforeResponse: true,
});

let acknowledged = false;

app.event('app_home_opened', async ({}) => {
await new Promise(resolve => setTimeout(resolve, 200));
assert.isFalse(acknowledged);
});

const event = {
body: {
type: 'event_callback',
token: 'XXYYZZ',
team_id: 'TXXXXXXXX',
api_app_id: 'AXXXXXXXXX',
event: {
type: 'app_home_opened',
event_ts: '1234567890.123456',
user: 'UXXXXXXX1',
text: 'hello friends!',
tab: 'home',
view: {},
},
},
respond: noop,
ack: async (_res: any) => {
acknowledged = true;
},
};

// Act
await fakeReceiver.sendEvent(event);

// Assert
assert.isTrue(acknowledged);
});

it('should call ack() after command listener completion', async () => {
// Arrange
const App = await importApp(overrides); // tslint:disable-line:variable-name
const fakeLogger = createFakeLogger();
const app = new App({
logger: fakeLogger,
receiver: fakeReceiver,
authorize: sinon.fake.resolves(dummyAuthorizationResult),
processBeforeResponse: true,
});

let acknowledged = false;

app.command('/hello', async ({ ack }) => {
assert.isFalse(acknowledged);
await ack();
});

const event = {
...baseEvent,
body: {
command: '/hello',
},
ack: async (_res: any) => {
acknowledged = true;
},
};

// Act
await fakeReceiver.sendEvent(event);
// Assert
assert.isTrue(acknowledged);
});
});
});
});
});
Expand Down
14 changes: 12 additions & 2 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ export default class App {
/** Receiver - ingests events from the Slack platform */
private receiver: Receiver;

/** Always call ack() after event listener completion if true */
private processBeforeResponse: boolean = false;

/** Logger */
private logger: Logger;

Expand Down Expand Up @@ -256,6 +259,7 @@ export default class App {
});
}
}
this.processBeforeResponse = processBeforeResponse;

// Conditionally use a global middleware that ignores events (including messages) that are sent from this app
if (ignoreSelf) {
Expand Down Expand Up @@ -580,8 +584,11 @@ export default class App {
if (type !== IncomingEventType.Event) {
listenerArgs.ack = ack;
} else {
// Events API requests are acknowledged right away, since there's no data expected
await ack();
// If processBeforeResponse is true, ack() call should be after the listener completion
if (!this.processBeforeResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice Fix!

// Events API requests are acknowledged right away, since there's no data expected
await ack();
}
}

// Get the client arg
Expand Down Expand Up @@ -635,6 +642,9 @@ export default class App {
} else if (rejectedListenerResults.length > 1) {
throw new MultipleListenerError(rejectedListenerResults.map(rlr => rlr.reason));
}
if (this.processBeforeResponse && type === IncomingEventType.Event) {
await ack();
}
},
);
} catch (error) {
Expand Down
22 changes: 17 additions & 5 deletions src/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,24 @@ export default class ExpressReceiver implements Receiver {

try {
await this.bolt?.processEvent(event);
if (storedResponse !== undefined) {
if (typeof storedResponse === 'string') {
res.send(storedResponse);
} else {
res.json(storedResponse);

if (this.processBeforeResponse) {
let spentMillis = 0;
const millisForTimeout = 10;
// Wait here until the isAcknowledged is marked as true or 3 seconds have passed
Copy link
Member Author

Choose a reason for hiding this comment

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

I know this is not ideal but I haven't figured out any other solutions yet.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused by this. Can you explain the error use case you are fixing here?

Copy link
Member Author

@seratch seratch Apr 1, 2020

Choose a reason for hiding this comment

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

@stevengill
I didn't mention clearly in the description (sorry!) but I have to say that processBeforeResponse just doesn't work for most cases (I haven't verified if it actually works in the code review, so that was not able to detect it 🙇 ). There are two issues with the current implementation as below:

  • processBeforeResponse doesn't respond to non-event requests such as commands (listeners are executed but no HTTP response is submitted - resulting in timeouts)
  • For Events API, processBeforeResponse doesn't wait for the completion of listeners and responds to incoming requests immediately (= it's not working as the solution for FaaS compatibility)

The change here is addressing the latter one both.

This fix surely holds off responding until verifying the acknowledgment on the App's processEvent method side. It works. However, I'm a bit hesitant to go with this "sleep" approach in Node.js. The most mysterious behavior I haven't fully understood yet is that await this.bolt?.processEvent(event); doesn't wait for the completion of listeners. The line returns before the completion of const settledListenerResults = await allSettled(listenerResults) in the method. That's why I had to implement this part in the Receiver side. I'm still trying to get rid of this part. It'd be greatly appreciated if you could dive deep into the issue.

Copy link
Member Author

@seratch seratch Apr 1, 2020

Choose a reason for hiding this comment

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

One additional note: I've added a unit test verifying App's behavior with processBeforeResponse. But I haven't added any tests for ExpressReceiver. So, even if all the unit tests have passed, it doesn't mean the behavior is valid. It's necessary to make sure if it works in real apps. I was going to add tests to the ExpressReceiver side as well but if we could get rid of this "sleep" part, probably it is no longer needed.

while (!isAcknowledged && spentMillis < 3000) {
await new Promise(resolve => setTimeout(resolve, millisForTimeout));
spentMillis += millisForTimeout;
}
if (isAcknowledged) {
this.logger.debug(`The listener execution completed in ${spentMillis} millis`);
if (typeof storedResponse === 'string') {
res.send(storedResponse);
} else {
res.json(storedResponse);
}
this.logger.debug('Acknowledged after the listener completion');
} // Otherwise, this Bolt app never responds to this request and above setTimeout outputs an error message
}
} catch (err) {
res.send(500);
Expand Down