-
Notifications
You must be signed in to change notification settings - Fork 73
Fix blockConcurrencyWhile
in DO queue
#674
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
Fix blockConcurrencyWhile
in DO queue
#674
Conversation
commit: |
|
||
if (this.ongoingRevalidations.size >= this.maxRevalidations) { | ||
if (this.ongoingRevalidations.size > 2 * this.maxRevalidations) { | ||
console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to import warning on line 1?
// We still await the promise to ensure the revalidation is completed | ||
// This is fine because the queue itself run inside a waitUntil | ||
await this.ctx.blockConcurrencyWhile(async () => { | ||
// TODO: need more investigation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you can have a minimal repro and create an issue on the workerd repro and link it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to work on that. It's a bit tricky to reproduce locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple comments
🦋 Changeset detectedLatest commit: bde2efd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
||
async revalidate(msg: QueueMessage) { | ||
if (this.ongoingRevalidations.size > 2 * this.maxRevalidations) { | ||
console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import warn?
Right now there is an issue with
blockConcurrencyWhile
that will make it block for 30s and fail.When there is too much revalidate request at a fast enough pace, it seems that there is multiple call to
blockConcurrencyWhile
and they seem to block each other.This PR fixes it by removing the
blockConcurrencyWhile
and just waiting forongoingRevalidation
to go belowmaxRevalidate