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.

Sign up for GitHub

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

Jump to bottom

Change WorkerInterface so prewarm returns a promise. #2863

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Change WorkerInterface so prewarm returns a promise. #2863

merged 3 commits into from
Oct 9, 2024

Conversation

danlapid
Copy link
Contributor

@danlapid danlapid commented Oct 8, 2024

No description provided.

@danlapid danlapid marked this pull request as ready for review October 8, 2024 20:39
@danlapid danlapid requested review from a team as code owners October 8, 2024 20:39
@anonrig
Copy link
Member

anonrig commented Oct 8, 2024

What's the reason for this change?

@danlapid danlapid mentioned this pull request Oct 8, 2024
Merged
@kentonv
Copy link
Member

kentonv commented Oct 9, 2024

Hmm, I vaguely remember someone tried this in the past and... something... went wrong... I know that's... not very helpful...

@danlapid
Copy link
Contributor Author

danlapid commented Oct 9, 2024

Hmm, I vaguely remember someone tried this in the past and... something... went wrong... I know that's... not very helpful...

Mike merged a very similar pull request in 2022 and encountered a segfault, it’s documented in the issue linked to the internal PR.
Harris believes that enough has changed in the codebase since that we can roll this out again and if we get any weird segfault we’ll just debug it and fix it.

@harrishancock
Copy link
Collaborator

What's the reason for this change?

Making prewarm() return a promise makes it easier for our internal code to track the ownership of asynchronous tasks, which affects how many threads we will allocate for a customer. There's an assertion that fires when we discover that we have lost track of the ownership of some background task in waitUntilTasks, and the prewarm promises recently started triggering this assertion when we enabled a new feature.

anonrig and others added 3 commits October 9, 2024 15:06
@anonrig @danlapid
add node:sys to comply with nodejs_compat
c094914
@danlapid
Change WorkerInterface so prewarm returns a promise.
4bded4c
@danlapid
Refactor waitUntilTasks from CustomEvent::sendRpc. (#2865)
603a2be 
Using waitUntilTasks in sendRpc is a footgun as you might depend on
a request to the rpc server that will happen complete after the request
has been responded to and the client closes the connection causing a
waitUntilTasks is not empty error. This PR and the internal PR fix any
remaining usages of waitUntilTasks and finally refactors waitUntilTasks
out of the code.
@danlapid
Copy link
Contributor Author

danlapid commented Oct 9, 2024

Last push is just a rebase

@jasnell
Copy link
Member

jasnell commented Oct 9, 2024

Let's be sure to get an internal CI run on this before merging. If you have already done so, just remove the label.

@danlapid danlapid merged commit b539fa5 into main Oct 9, 2024
14 of 15 checks passed
@danlapid danlapid deleted the dlapid/prewarm_promise branch October 9, 2024 17:01
@danlapid danlapid mentioned this pull request Oct 9, 2024
Merged
@danlapid danlapid restored the dlapid/prewarm_promise branch October 9, 2024 20:42
danlapid added a commit that referenced this pull request Oct 9, 2024
@danlapid
Merge pull request #2863 from cloudflare/dlapid/prewarm_promise
fb4e33f 
Change WorkerInterface so prewarm returns a promise.
danlapid added a commit that referenced this pull request Oct 10, 2024
@danlapid
Merge pull request #2863 from cloudflare/dlapid/prewarm_promise
f26e25f 
Change WorkerInterface so prewarm returns a promise.
danlapid added a commit that referenced this pull request Oct 11, 2024
@danlapid
Merge pull request #2863 from cloudflare/dlapid/prewarm_promise
1dae2d0 
Change WorkerInterface so prewarm returns a promise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@harrishancock harrishancock harrishancock approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danlapid @anonrig @kentonv @harrishancock @jasnell