-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: idle syncing #269
feat: idle syncing #269
Conversation
|
||
// https://developer.chrome.com/docs/extensions/reference/api/alarms | ||
void chrome.alarms.create('blockSync', { | ||
periodInMinutes: 30, |
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.
dynamically adjustable param
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.
FWIW, I think it will have to be much shorter than this (between 20 seconds and 4 minutes)
https://developer.chrome.com/docs/extensions/develop/concepts/service-workers/lifecycle
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 don't totally understand what causes the service worker to terminate, if it's long-polling the compact block stream or real idling.
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.
FWIW, I think it will have to be much shorter than this (between 20 seconds and 4 minutes)
alarms can now be set to a minimum period of 30s to match the service worker lifecycle: "starting in Chrome 120, the minimum alarm interval has been reduced from 1 minute to 30 seconds"
I've been testing locally with 1 minute.
One test that comes to mind is, with each version of the code (pre and post change):
(a custom chrome profile may be helpful here) If background syncing is working we expect the post-change test to have significantly higher sync height at (6). Exactly how much is less important — as long as it's increasing we have reason to believe it will catch up and stay up to date. |
per the docs: "starting in Chrome 117, the number of active alarms is limited to 500. Once this limit is reached, chrome.alarms.create() will fail. When using a callback, chrome.runtime.lastError will be set. When using promises, the promise will be rejected." seems like a necessary extension to this might be to periodically clear all alarms using |
i considered alarms unnecessary because every content script (meaning every page load) will attempt to message the extension background with an init message, to connect pre-authorized origins. this message should wake up the extension background. the init process changed a lot and things are scoped differently now, so probably at this point it just wakes up enough scope to handle the message listener callback, which doesn't access services. meaning service init is actually skipped. the simplest way to handle this with alarms is, create an alarm with
it should be possible to refactor this in a way that avoids alarms and any new alarm permission, by refactoring the init message handler to require the return value of |
hey!!
...
so the but, the use case we're targeting is when the user doesn't open the extension and load a frontend for prolonged periods, which wouldn't trigger any content script init messages, and thus make no sync progress while the user's been offline.
thought the same thing initially, but in practice just creating the alarm is sufficient to wake up the service worker and kickstarting init - seems we don't need to put
yeah, CRSessionManager's singleton pattern prevents multiple session managers, so subsequent calls just return the existing sessions map. alarms are a nice idiomatic solution for periodically waking up the service worker. I do wonder however how the additional permissions will impact chrome store reviews. |
an alarm is definitely the idiomatic way to do it. i expect reviewers may take issue with a high frequency. worker kill conditions seem deliberately under-documented, and the conditions explained (pending fetch, pending promise resolution) are definitely something that the present behavior should meet. so at some point it's killed despite meeting the no-kill conditions. i would investigate how long the worker runs if it's activated by the alarm but conducts no other activity, and experiment with setting an alarm
to identify the lowest effective frequency. even if there is a significant kill time between, if it can catch up before the next kill, the frequency can be reduced. then document and justify the alarm reason as much as possible for the reviewers and users. |
@hdevalence replicated the procedure on my older 2020 m1 pro – the browser cache was cleared, and neither any frontends nor the extension were opened. Screen.Recording.2025-01-22.at.4.18.52.PM.movafter starting chrome and waiting 10 minutes, it synced the entire chain (with alarms firing every minute). Previously, it would have just blocked on genesis sync during service worker init without making any progress. Screen.Recording.2025-01-22.at.4.29.37.PM.movBETA TESTERS will replicate this waiting 1 hour instead. |
Well, if the alarm approach solves the sync problem, that seems like a good improvement, and we could continue to refine it over time. A 30 minute alarm seems low frequency but we can find out if there are objections. |
note to reviewers: this will be staged in #263, cut as part of the next prax |
apps/extension/src/service-worker.ts
Outdated
|
||
chrome.alarms.onAlarm.addListener(alarm => { | ||
if (alarm.name === 'blockSync') { | ||
console.log('Block sync alarm fired, service worker restarting'); |
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.
for checking beta reviewer logs
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.
This shows up in the service worker console right? not the main web one
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.
logs display in both since chrome forwards the service worker logs to the main extension console
chrome forwards the service worker logs to the main extension console, but it won't show up in the web browser's console.
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.
Cool, good work. I think we should get this in the hands of beta testers asap. The wake up frequency seems polite and reasonable so I hope Chrome review won't object.
uses chrome.alarms API to restart the service worker after extended periods of inactivity and make sync progress over time without any frontend providers or the extension being opened. This ensures the chain is already synced when the user returns to penumbra after that extended period of not using it. We aptly name this 'idle syncing'.
(this a long winded sanity check for sidestepping a major refactor to our connection lifecycle)
cc @hdevalence
when adding an event listener to log details as the alarm triggers (set
periodInMinutes
to 1 minute) and inspecting the service worker immediately after manually terminating it withchrome.runtime.reload()
, it appears to working at first glance. However, we should contemplate potential pitfalls with this approach.see #269 (comment) for an example