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

feat: Use notify push for sync messages during editing #4585

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Jul 26, 2023

Signed-off-by: Julius Härtl jus@bitgrid.net

📝 Summary

This is a first PoC to make use of notify_push for syncing steps back to other editors instead of regular polling. We still poll every 30 seconds as a fallback and to ensure that the autosave requests are still triggered until #4424 is there (even with that some fallback sync might still be worth).

🚧 TODO

  • More manual testing in different scenarios
    • Check what happens with the base doc changing externally (did we use the sync to detect the conflict?)

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Jul 26, 2023

1 flaky tests on run #11336 ↗︎

0 149 2 0 Flakiness 1

Details:

feat: Use notify push for sync messages during editing
Project: Text Commit: 90c956236b
Status: Passed Duration: 04:17 💡
Started: Jul 27, 2023 6:27 PM Ended: Jul 27, 2023 6:32 PM
Flakiness  cypress/e2e/nodes/Links.spec.js • 1 flaky test

View Output Video

Test Artifacts
... > Link website with selection Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

private function addToPushQueue(Session $session, Document $document, int $version): void {
try {
$queue = Server::get(IQueue::class);
$syncResponse = $this->sync($session, $document, $version);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect to just add the steps that just arrived. Say we start at version 0 and two users push their steps without syncing in between:

  • user1 pushes step1
  • user2 pushes step2

Now the first push will add [step1] and the second push will add [step1, step2] to the queue because user2 still has version 0.

Or in other words.. the content of the queue gets send to all users and what is added to it should not depend on the version of the user adding it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely, so we probably need some sync calls to still happen to catch up in the beginning and maybe some kind of detection of missing steps (if we can)?

Otherwise maybe there is a way to make use of the y.js native catch up https://github.com/yjs/y-protocols/blob/master/sync.js#L15-L17

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y-websocket has a timer that will send Sync messages of type 1 periodically:
https://github.com/yjs/y-websocket/blob/35be3431b8db14f07dfaacab5dfa1a4cdec13fac/src/y-websocket.js#L300-L311

We could enable that. But if the frequency is too high it will create quite a bit of load as all clients would query all other clients. If the frequency is too low it will take too long to resync to be helpful.

My favorite approach right now would be for the saving client to also save a Sync message type 1 alongside the current state. Then every client can get that as a baseline and apply the saved state and reply to the message type 1 with a message type 2 that will contain its local diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be resolved by just using the notify_push queue as a fast path to distribute any incoming messages to other and the own clients.

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Jul 28, 2023

Thanks a lot for looking into this. I'm really curious to play with it. 😍

We still poll every 30 seconds as a fallback and to ensure that the autosave requests are still triggered until #4424 is there (even with that some fallback sync might still be worth).

Just one remark. Autosave is independent from the polling backend these days. It's triggered by

this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL)

Never the less I agree that having a fallback for a start might still be worthwhile.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using notify_push for syncing
2 participants