Re-fix #395 by adjusting the processBeforeResponse option #459
Conversation
seratch
added
the
bug
Codecov Report
@@ Coverage Diff @@
## master #459 +/- ##
==========================================
- Coverage 84.72% 83.65% -1.07%
==========================================
Files 7 7
Lines 550 563 +13
Branches 163 167 +4
==========================================
+ Hits 466 471 +5
- Misses 57 65 +8
Partials 27 27
Continue to review full report at Codecov.
|
| if (this.processBeforeResponse) { | ||
| let spentMillis = 0; | ||
| const millisForTimeout = 10; | ||
| // Wait here until the isAcknowledged is marked as true or 3 seconds have passed |
There was a problem hiding this comment.
I know this is not ideal but I haven't figured out any other solutions yet.
There was a problem hiding this comment.
I'm a little confused by this. Can you explain the error use case you are fixing here?
There was a problem hiding this comment.
@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:
processBeforeResponsedoesn't respond to non-event requests such as commands (listeners are executed but no HTTP response is submitted - resulting in timeouts)- For Events API,
processBeforeResponsedoesn'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.
There was a problem hiding this comment.
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.
| // 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) { |
| if (this.processBeforeResponse) { | ||
| let spentMillis = 0; | ||
| const millisForTimeout = 10; | ||
| // Wait here until the isAcknowledged is marked as true or 3 seconds have passed |
There was a problem hiding this comment.
I'm a little confused by this. Can you explain the error use case you are fixing here?
|
update: i'm still looking into a fix for the underlying issue here. we've narrowed that down to the i'm starting by producing a test case for |
aoberoi
removed
the
bug
Summary
This pull request fixes #462 by correcting the #444 implementation to work at any case.
Requirements (place an
xin each[ ])