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.

Sign up for GitHub

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

Jump to bottom

Keep base version etag during reload #5941

Merged
merged 7 commits into from
Jul 3, 2024
Merged

Keep base version etag during reload #5941

merged 7 commits into from
Jul 3, 2024

Conversation

max-nextcloud
Copy link
Collaborator

📝 Summary

this.$syncService is cleared during the close method.

However we need the baseVersionEtag to ensure the editing session
on the server
is still in sync with our local ydoc.

See #5724 (comment) for a trace of the effect of this.

Also fixes #5726

🏁 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 is not required

@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Jun 25, 2024
@juliusknorr juliusknorr added this to the Nextcloud 30 milestone Jun 25, 2024
@max-nextcloud max-nextcloud self-assigned this Jun 27, 2024
@max-nextcloud max-nextcloud marked this pull request as draft June 27, 2024 12:01
@max-nextcloud
Copy link
Collaborator Author

I'm looking into the flaky sync tests and fixing some issues with lost network connectivity on the way.
In particular when clicking reconnect the editor and the status message go away now. If the reconnect fails there's nothing one can do anymore.
For now I'll focus on just keeping the status message so one can try to reconnect multiple times. Keeping the editor would be great but seems more involved.

@max-nextcloud max-nextcloud force-pushed the fix/keep-base-version-etag-during-reload branch from df1131e to dab63df Compare July 1, 2024 07:58
@max-nextcloud max-nextcloud marked this pull request as ready for review July 1, 2024 12:04
juliusknorr
juliusknorr approved these changes Jul 1, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Very nice, thanks also for the nicely split commits. Makes reviewing a very straight forward ❤️

src/services/SyncService.js Outdated Show resolved Hide resolved
@max-nextcloud
fix(sessionApi): only mark api as closed on success
98cb9bf 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(reconnect): keep baseVersionEtag during reconnect
6093128 
`this.$syncService` is cleared during the `close` method.

However we need the `baseVersionEtag` to ensure the editing session
on the server
is still in sync with our local ydoc.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
refactor: hide connection in sync service
a4ea9fc 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
test(cy): wait longer for initial sync to avoid timeouts
eb9dcbb 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
test(cy): also test failed reconnect attempt
97da812 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(Editor): separate close and disconnect functions
e214f6f 
* `close` is for closing the editor.
  It tries to save the document and clean everything up.
* `disconnect` is for cleaning up the current collaboration sessions.
  It will not save the document and asumes the editing will be resumed
  after a reconnect.

Move `sendRemainingSteps` out to the sync service.
Also make close in the websocket polyfill sync.
Just clean up the polyfills state.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
refactor(yjs): move queue handling into helper
13c580c 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the fix/keep-base-version-etag-during-reload branch from e056263 to 13c580c Compare July 2, 2024 07:11
@max-nextcloud
Copy link
Collaborator Author

/backport to stable29

@max-nextcloud
Copy link
Collaborator Author

/backport to stable28

mejo-
mejo- reviewed Jul 2, 2024
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Did some testing and seems to work well. Thanks so much for looking into it.

When cutting the network and pressing "Reconnect", we run into

sendStepsNow(): this.connection is undefined because the connection got closed but sendRemainingSteps() still tries to run sendStepsNow().

In the same scenario Error: Close has already been called on the connection is also still logged.

@max-nextcloud
Copy link
Collaborator Author

When cutting the network and pressing "Reconnect", we run into

sendStepsNow(): this.connection is undefined because the connection got closed but sendRemainingSteps() still tries to run sendStepsNow().

In the same scenario Error: Close has already been called on the connection is also still logged.

I tried but failed to reproduce these two.

I also don't see how that would happen as reconnect should not send the remaining steps anymore. But I will take a look at the code to confirm.

max-nextcloud
max-nextcloud commented Jul 3, 2024
View reviewed changes
src/components/Editor.vue
Comment on lines -435 to +433
this.close().then(this.initSession)
this.disconnect().then(this.initSession)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reconnect calling disconnect rather than close now.

max-nextcloud
max-nextcloud commented Jul 3, 2024
View reviewed changes
src/components/Editor.vue
Comment on lines +663 to +672
async disconnect() {
await this.$syncService.close()
this.unlistenSyncServiceEvents()
this.$providers.forEach(p => p?.destroy())
this.$providers = []
this.$syncService = null
// disallow editing while still showing the content
this.readOnly = true
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no sendRemainingSteps here.

@max-nextcloud max-nextcloud merged commit cae237a into main Jul 3, 2024
59 of 61 checks passed
@max-nextcloud max-nextcloud deleted the fix/keep-base-version-etag-during-reload branch July 3, 2024 10:20
@backportbot backportbot bot mentioned this pull request Jul 3, 2024
Merged
2 tasks
@max-nextcloud
Copy link
Collaborator Author

Merged as i could not reproduce the remaining issue. @mejo- Please open a new issue based on the comment if this still happens for you.

@backportbot backportbot bot mentioned this pull request Jul 3, 2024
Merged
2 tasks
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

Assignees

@max-nextcloud max-nextcloud

Labels
2. developing bug Something isn't working
Projects
📝 Office team
Archived in project
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

Close has already been called on the connection
3 participants
@max-nextcloud @juliusknorr @mejo-