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

fix: Do not overwrite current local version on reconnect #5082

Closed
wants to merge 1 commit into from

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Dec 4, 2023

Experimenting a bit with reconnect behaviour i noticed an issue that might cause disconnected session states.

Two separate sessions A and B

  • A and B are editing
  • A network goes offline (sometimes i needed to also quickly type something as A after setting the network offline)
  • B continues typing
  • A waits until the first auto close request gets blocked in the network console
  • A network goes online again
  • Reconnect happens

Now I observed that the last sync message had a different version to request steps compared to the first of the new session which turned out to be the case because we always overwrite the version with the docStateVersion on opening a session in the frontend, however in the reconnect case we already have a different state as we do not start from scratch.

Contributes to #4943

  • Check if there is a full recreate approach possible as a quick solution
  • Just refetching the steps should not have any effect here as the new document state should already contain those
    • Maybe it is more about the local steps that then conflict with the new document state?
  • check how applying the document state influences the existing y.js doc
  • Figure out some visual indicator for beta versions if something goes wrong and one should report the browser console logs

🏁 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

@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Dec 4, 2023
@juliusknorr juliusknorr added this to the Nextcloud 29 milestone Dec 4, 2023
@juliusknorr
Copy link
Member Author

To check additionally if the local document state gets somehow set still on reconnect as at least the changes from other still online clients occur fine

@juliusknorr
Copy link
Member Author

To check additionally if the local document state gets somehow set still on reconnect as at least the changes from other still online clients occur fine

Oh indeed, sync service emits loaded the document state is just applied in

this.hasConnectionIssue = false

@max-nextcloud I'm wondering if that could cause additional troubles if we already have an existing YDoc? We may just want to skip this then and fully rely on catching up with steps during sync.

Additionally we may want to saveguard this to actually do a full reload if the yjs history has been reset in the meantime I guess.

@max-nextcloud
Copy link
Collaborator

If I remember correctly my thinking was... the doc state will include all changes to the document up to the version send out during the (re-)connect. So if the client applies them it's up to sync until that version and can start from there.

If this is still the same session I don't see why this would not work. If it's a different session then we'd really need to clear the local state completely and then reconnect.

@juliusknorr
Copy link
Member Author

As discussed let me debug this further as I think this change should not have any effect on the bug behaviour. I'll close this for now and will continue in #4943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants