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

Add compat flag for queue consumers to not block on waitUntil'ed work #3596

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a-robinson
Copy link
Member

The intent is that the event can be considered done when the
queue() handler's returned promise resolves, allowing acked/nacked
message info to be returned back to the queues infrastructure sooner,
rather than requiring all waitUntil'ed work to finish first. This has
two important benefits:

  1. This will keep consumer invocations from getting slowed down by
    slow/hung waitUntil'ed tasks.
  2. This will keep consumer invocations from failing due to failing
    waitUntil tasks.

This notably makes queue() handlers behave more like fetch() handlers, which should make them more intuitive for most users.

@a-robinson a-robinson requested review from a team as code owners February 24, 2025 15:37
@a-robinson a-robinson requested review from fhanau and tewaro February 24, 2025 15:37
@jasnell
Copy link
Member

jasnell commented Feb 24, 2025

@fhanau ... this will likely impact the scheduling of the Outcome event for streaming tail workers.

@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch from f97c4f7 to e207648 Compare February 24, 2025 21:16
@a-robinson
Copy link
Member Author

Fixed up the code to implement the decisions for the two questions above. There's a lot of test coverage in the corresponding internal PR, so from my perspective this is good to go.

@a-robinson
Copy link
Member Author

a-robinson commented Feb 24, 2025

Ughh the linter doesn't like something about the formatting -- forgive any small force pushes for a little bit while I try to fix it since I guess the version of the formatter that I'm running locally isn't up to date anymore, but I've never been able to get the tooling in this repo to work :|

@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch from e207648 to c96dc1e Compare February 24, 2025 21:28
@a-robinson
Copy link
Member Author

Ughh the linter doesn't like something about the formatting -- forgive any small force pushes for a little bit while I try to fix it since I guess the version of the formatter that I'm running locally isn't up to date anymore, but I've never been able to get the tooling in this repo to work :|

Ok, looks like it was just the one line that the linter was unhappy with -- it's passing now.

So this just needs review from someone on the runtime team, since @jbwcloudflare doesn't count.

@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch from c96dc1e to e462427 Compare February 24, 2025 22:53
The intent is that the event can be considered done when the
queue() handler's returned promise resolves, allowing acked/nacked
message info to be returned back to the queues infrastructure sooner,
rather than requiring all waitUntil'ed work to finish first. This has
two important benefits:
1. This will keep consumer invocations from getting slowed down by
   slow/hung waitUntil'ed tasks.
1. This will keep consumer invocations from failing due to failing
   waitUntil tasks.

This commit does not implement the new functionality, just adds the flag
and adds the logic to read the flag where needed. The actual
implementation will come in the next commit for easier review.
@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch 2 times, most recently from 18a6a4c to a796f1f Compare February 25, 2025 15:57
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like to see some additional code comments explaining the code flow a bit more. With the promise chains it becomes a bit obtuse very quickly

@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch from f630f72 to 673dec1 Compare February 25, 2025 17:36
@a-robinson
Copy link
Member Author

Thanks for the review!

@a-robinson a-robinson force-pushed the arobinson/queue-waituntil branch from 673dec1 to 9cd8c0f Compare February 25, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants