seratch / slack-edge

Slack app development framework for edge functions with streamlined TypeScript support
https://github.com/seratch/slack-edge-app-template
MIT License
87 stars 5 forks source link

[Discussion] Lazy vs Ack Race Condition #11

Closed zhawtof closed 8 months ago

zhawtof commented 8 months ago

Hey @seratch, we've recently run into a section of your code that we believe might have a better solution.

https://github.com/seratch/slack-edge/blob/21f917867b4e3313c7289601b053b210e02abeaf/src/app.ts#L667-L668

In the above code, the lazy function is triggered before the ack function. While this might be intended, we believe that the lazy function would be substantially more useful if it was triggered after the ack function.

Our thoughts in no particular order:

  1. If lazy and ack both access and update the same information, this will cause a race condition.
  2. lazy by its name implies that it would follow Lazy evaluation and only be triggered when resources are available. By triggering before ack, it still hogs resources that should be used for ack even if the processing is parallel.
  3. It's far less useful than a lazy function after the ack. A lazy function at the end of a time-sensitive ack could perform operations that are unnecessary for client resolution.

Real example

At the very onset of our product onboarding, we create an object that represents our platform. After our platform is created, we wanted to perform follow-on tasks that would continue preparing the rest of the platform behind the scenes. However, because lazy evaluates before ack evaluates, we cannot run any follow-on tasks since there is a race condition that the platform has yet to be created.

Our current solution

Our solution (which I will add is perfectly reasonable) is to pass down the Cloudflare ExecutionContext and run waitUntil within the ack directly. We mention the suggestions because we believe it would improve the slack-edge platform by abstracting Cloudflare capabilities.

Our suggestion

1) Swap the order of ack and lazy

const slackResponse = await handler.ack(slackRequest); 
ctx.waitUntil(handler.lazy(slackRequest)); 

2) Introduce 3 commands. preAck, ack, postAck.

ctx.waitUntil(handler.preAck(slackRequest)); 
const slackResponse = await handler.ack(slackRequest); 
ctx.waitUntil(handler.postAck(slackRequest)); 

Would love to hear your thoughts! We love the platform and we're excited to scale with it!

seratch commented 8 months ago

Hi @zhawtof, thanks for the input. I personally don't call this a race codition, but I do understand your need here. As for your suggestions:

I pefer the 2. approach but instead of two types of lazy listeners, I'd like to add a new initizaliation option to customize the behavior called startLazyListenerAfterAck: boolean | undefined (undefined is the default value and it means false). This approach does not bring any breaking changes to existing apps. You can pass startLazyListenerAfterAck: true to the App constructor. Will submit a PR for it.

seratch commented 8 months ago

Thank you so much again for your input! v0.9.0 is now available with the new option.