Skip to content

fix: load fresh session if none are remaining #3890

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

Merged
max-nextcloud merged 1 commit into from
Mar 4, 2023
Merged

fix: load fresh session if none are remaining #3890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cypress/e2e/SessionApi.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,5 +349,16 @@ describe('The session Api', function() {
})
})

// Failed with a probability of ~ 50% initially
it('ignores steps stored after close cleaned up', function() {
cy.pushAndClose({ connection, steps: [messages.update], version })
cy.createTextSession(undefined, { filePath: '', shareToken })
.then(con => {
connection = con
})
.its('state.documentSource')
.should('eql', '## Hello world\n')
})

})
})
11 changes: 11 additions & 0 deletions cypress/support/sessions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,14 @@ Cypress.Commands.add('syncSteps', (connection, options = { version: 0 }) => {
return connection.sync(options)
.then(response => response.data)
})

// Used to test for race conditions between the last push and the close request
Cypress.Commands.add('pushAndClose', ({ connection, steps, version, awareness = '' }) => {
cy.log('Race between push and close')
.then(() => {
const push = connection.push({ steps, version, awareness })
.catch(e => e) // handle 403 gracefully
const close = connection.close()
return Promise.all([push, close])
})
})
6 changes: 3 additions & 3 deletions lib/Service/ApiService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,12 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
$readOnly = $this->documentService->isReadOnly($file, $token);

$this->sessionService->removeInactiveSessions($file->getId());
$activeSessions = $this->sessionService->getActiveSessions($file->getId());
$remainingSessions = $this->sessionService->getAllSessions($file->getId());
$freshSession = false;
if ($forceRecreate || count($activeSessions) === 0) {
if ($forceRecreate || count($remainingSessions) === 0) {
Copy link
Member

@mejo- mejo- Mar 4, 2023

Choose a reason for hiding this comment

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

Not sure I understand the impact of this change: we only create a fresh session now if "no session was found", instead of "no active session was found" before, right? So we will always try to restore sessions, even if they're not active anymore?

Copy link
Collaborator Author

@max-nextcloud max-nextcloud Mar 4, 2023

Choose a reason for hiding this comment

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

Yes. Sessions become inactive after 5 minutes. But there may still be steps that were created during the session but never persisted. All the cleanup functions will leave those steps and their session in place (unless forceReload is set). This change makes it so we still recover these changes after more than 5 minutes.

$freshSession = true;
try {
$this->documentService->resetDocument($file->getId(), $forceRecreate);
$freshSession = true;
Copy link
Member

@mejo- mejo- Mar 4, 2023

Choose a reason for hiding this comment

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

If I understand correctly, this means that we always start a fresh session here, also if resetDocument() throws a DocumentHasUnsavedChangesException, right? Could this lead to data loss of the unsaved changes? Probably we cannot do anything about it anway, as there is no session to apply the changes to anymore, right?

Copy link
Collaborator Author

@max-nextcloud max-nextcloud Mar 4, 2023

Choose a reason for hiding this comment

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

You're right - that's exactly what this change is aiming for. Let me explain the reasoning in more detail.

We would not need the session to apply the steps - they are self contained - as long as all of them are present.

However if the session does not exist anymore it must have been removed. If steps are still present this only happens during a close call. So close was called on the session.

If other sessions are still around we won't run into this block anyway. If there were other sessions around during the close they would have been closed as well and would have cleaned up the steps. Therefore if we end up here the session must have been the last active session when it was closed. In that case it will have cleaned up it's steps and the remaining unsaved changes are just leftovers from the race condition and yes... we cannot apply them anymore because we lack the history leading up to them.

} catch (DocumentHasUnsavedChangesException $e) {
}
}
Expand Down