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

Proposal: Sync in worker #1041

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

psrpinto
Copy link
Contributor

@psrpinto psrpinto commented Feb 23, 2023

Addresses #1045

This is a proposal for running sync in a worker, so that it would be possible to have the same session open in multiple tabs/windows/iframes.

Rendered

TODO

  • Clarify the lifetime and ownership of workers
    • Take in account that service worker must remain optional
    • Explore possibility of using SharedWorker
  • Clarify whether mutations to the Session in the main thread need to be communicated to the worker
  • Clarify how messages will be passed between main thread and worker, check browser support for BroadcastChannel.
  • Clarify whether there's a limit on the number of workers that can run at a given time
  • Clarify the usage of localStorage in Storage, since localStorage is not available in workers
  • Clarify "after sync" step
    • Some things should only be run once per sync
    • List of things that would need their updates passed over the message channel between the worker and the page

@psrpinto psrpinto marked this pull request as ready for review February 23, 2023 17:23
@bwindels
Copy link
Contributor

Thanks for this proposal, some early thoughts:

  • being able to build workers with vite would be useful indeed, you might find some notes in Allow SDK usage from service worker #292 thay may or may not be outdated.
  • given the scope of these changes, it would be absolutely critical that we can do this behind a feature flag (apart from any build-system changes perhaps).
  • putting sync in a worker won't help us having to elect a master tab, as a worker's lifetime is tied to the page that started it. A service worker doesn't have this limitation though, and putting sync in the service worker could make sense (and have some benefits like being able to sync when a web push notification comes in) but I'd be a bit hesitant to make Hydrogen not being able to run at all without a service worker. We probably want to be flexible here and allow for different strategies.
  • Something worth mentioning: the afterSyncCompleted (on room and session) step should only be run once per sync request. In this sync step, we'll send out network requests that need to happen as a side-effect of the sync, like sharing keys, backfilling, ... so this should only happen once.
  • Some things probably should not be mirrored on both pages, like when you start a call.

@bwindels
Copy link
Contributor

I just saw you mentioned the worker being owned by the service worker. That has the same limitation that hydrogen wouldn't be able to run without a worker though.

A SharedWorker seems like what we want, but support is lacking on mobile platforms. Perhaps that's good enough though, use a service worker in production, and a SharedWorker during development time. We should of course still be able to run sync within the page and disable cross-tab syncing.

@bwindels
Copy link
Contributor

A good first step would be to list things of things that would need their updates passed over the message channel between the worker and the page:

  • room summary updates
  • timeline updates
  • need to think if there's anything else

@psrpinto
Copy link
Contributor Author

Thanks for looking into this proposal @bwindels!

Agree, we would be making the changes behind a feature flag, which would allows us to spread the implementation across multiple PRs, and iterate on the feature until we're confident that it's stable.

When the feature flag is disabled, or the environment doesn't allow for running sync in a worker (e.g. no support for Web Workers API), we would fallback to the non-worker sync, exactly as is today. If necessary, we could also make it possible to disable sync-in-worker through runtime config, or even at the build level.

I agree Hydrogen must continue to be functional without a service worker. I will look further into this topic, and into the "after sync" step and will update the document accordingly.

I've updated the PR description with a TODO section.

@psrpinto
Copy link
Contributor Author

list things of things that would need their updates passed over the message channel between the worker and the page

@bwindels Is the opposite scenario also needed, i.e. passing updates to the worker when the Session in the page is modified? Looking at the architecture docs it's mentioned that:

Sync is the thing (although not the only thing) that mutates the Session

So if mutations to the Session happen in the main page, would the worker also need those mutations for Sync to work correctly? I will look further into this but figured you might know something from the top of your head.

@bwindels
Copy link
Contributor

list things of things that would need their updates passed over the message channel between the worker and the page

@bwindels Is the opposite scenario also needed, i.e. passing updates to the worker when the Session in the page is modified? Looking at the architecture docs it's mentioned that:

Sync is the thing (although not the only thing) that mutates the Session

So if mutations to the Session happen in the main page, would the worker also need those mutations for Sync to work correctly? I will look further into this but figured you might know something from the top of your head.

hmm, you could probably get away with relying on remote echo, e.g. the /sync api reflecting a change you made like sending a message, creating a room, ... there will be a delay for the server roundtrip but that should be ok?

@psrpinto
Copy link
Contributor Author

Relying on sync to propagate the changes makes sense, server roundtrip should be fine indeed.

@psrpinto
Copy link
Contributor Author

psrpinto commented Mar 2, 2023

I've added a couple of sections to the doc (Worker lifetime and Gracefully closing sessions) that address the discussions above about service worker vs shared worker, and other topics.

In short, we propose to use a "double worker" solution, with one SharedWorker, which then spawns a sync Worker per sessionId (please see the doc for the reasoning behind this). Looking forward to hearing your thoughts on this @bwindels.

I've also created #1045 to track the feature, which would also serve as the epic for this endeavour. I've started extracting actionables from this proposal into a TODO section on that issue. Let me know if there's anything in that TODO that seems off, or if you have any other feedback.

In the meantime, I'm looking into the other topics in the TODO of this PR.

@psrpinto
Copy link
Contributor Author

psrpinto commented Mar 3, 2023

I just realised that there are some things that aren't really clear in the Worker lifetime and Gracefully closing sessions so I'm giving it another pass. Will update here once done.

@psrpinto
Copy link
Contributor Author

psrpinto commented Mar 6, 2023

@bwindels I've clarified the service worker vs shared worker topic in the doc.

In short, there's one SharedWorker per session:

const sessionId = "foo";
const worker = new SharedWorker(new URL('./sync-worker.js'), {
    name: `sync-worker-${sessionId}`,
})

This code can run in multiple windows/tabs/iframes. The browser spawns a worker the first time one is created with the given name. Subsequent attempts to create a worker with the same name result in the existing one being returned. The browser will despawn the worker once there are no more references to it.

@bwindels
Copy link
Contributor

bwindels commented Mar 16, 2023

Just saw that workers of type es module are coming to firefox! This means we may not need a build-step to convert import to importScripts for the SDK, depending on which browsers need to support the tab sharing feature! it's still behind a flag, but one would hope they enable it soon enough by default. See https://caniuse.com/mdn-api_worker_worker_ecmascript_modules and https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

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.

None yet

2 participants