-
Notifications
You must be signed in to change notification settings - Fork 26
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
RFC: Implement clean shutdown and in-memory document GC #233
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
@rbtying is attempting to deploy a commit to the drifting-corp Team on Vercel. A member of the Team first needs to authorize it. |
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.
Thanks for this! I like the use of the connection token, that seems like a good fit here.
If you rebase on main, the integration tests should pass -- they were breaking because of an unrelated cloudflare workers version issue.
When the server receives a ctrl-c, stop taking new requests, immediately trigger a save on all open documents, wait for all the documents to persist, and then exit.
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.
Looks good, thanks for this! One question/comment, and then I'll merge.
crates/y-sweet/src/server.rs
Outdated
.layer(middleware::from_fn_with_state( | ||
redact_errors, | ||
Self::redact_error_middleware, | ||
)); |
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 like the middleware approach here, but since the redact error middleware is a no-op if redact_errors = false
, should we just conditionally not add the middleware in that case and avoid passing through the boolean state?
Unfortunately it looks like yrs is not thread-safe (y-crdt/y-crdt#373), so I might tinker a bit to see if there's a way to do the yrs subscription drops single-threaded before merging |
I think this is unblocked w/ the 0.19 update! |
Merged! Thanks for all your work on getting this across the finish line! |
I noticed in #228 that the y-sweet binary is currently missing at least best-effort persistence-before-shutdown, and GCing documents out of memory. I understand that y-sweet is currently mostly deployed as CloudFlare workers, so both of those are a bit moot - CloudFlare will kill the process entirely once there aren't any connections for long enough -- but I figured that it might be nice to have something for outside-CloudFlare as well.
This PR implements:
y-sweet-core
with implementation details of the server. The garbage collection task holds the cancellation token for the persistence worker, so in graceful shutdown scenarios it should always persist the current state.