Fix: Stop rebasing updates after they come in #1599
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See #1579, #1598 See Simperium/node-simperium#78 See Simperium/node-simperium#61 When we receive an update from a remote client we have been listening for it and adjusting our local app state to account for that change. Unfortunately in cases where we also have local or pending changes we have been erroneously transforming or _rebasing_ the change before applying it. The underlying `node-simperium` library has already performd that transform, however, and when the client application does that too it leads to double-writes and misapplied patches, producing note corruption. In this change we're stopping the rebase operation we have been performing and that will remove this particular bug from the application. This change does not solve all of the problem however because we also have to make sure that the `node-simperium` library is aware of our local state when it receives those updates. Further work is taking place in Simperium/node-simperium#61 and #1598 to close the loop on that but these changes are important enough on their own to warrant a change. This will close one bug while opening another but this is a dependent part of the process in closing the broader issue.
dmsnell
added a commit
that referenced
this pull request
Nov 16, 2019
Resolves #1690 Obviates session lock in: #1700, #1704, #1707, #1710, #1720 Resolves bug that uncovered signout/signin issue: #1664 Follows Simperium API change in: #1598, #1599, Simperium/node-simperium#61, Simperium/node-simperium#78 When we fixed some deep and underlying issues in `node-simperium` we started a chain of operations which had to adjust to that change. Remember that the core problem was an assumption that after we send out a change that we could wait until it came back. That assumption was wrong because changes from other remote clients could come in during that time period while we're waiting. Simperium/node-simperium#61 fixed that problem but because it was broken in the past we added some work-arounds in #706; these work-arounds complicated the flow of data in the corrected version so we had to remove them and change how we handle remote updates in the Simplnote side. A critical step in this flow is letting Simperium know what local changes we may have in the app buffer. We introduced this step in #1598 but never noticed a typo: ```js getState().selectedNoteId ``` That reference will always be wrong and so we'll never send the local contents of the note. It should have been... ```js getState().appState.selectedNoteId ``` The implication of this change is that we always told Simperium that there are no local changes and so it would grab the currenty copy in the bucket, which in our case is in shared `indexedDB` tables, and thus it would occasionally get updated copies of the bucket that were updated in another session. This would then appear as though we also made the same change we were receiving, and thus we would prepare our own patch to send out and start the cycle. Further we were only sending local updates in the case that we had the same note open as was updated in the remote change. However, since we don't update Simperium with notes that we're not editing we were leaving other notes behind by sending `null` for them. The outcome of this is that edits to notes that aren't selected would have a modification in the way they are applied, if otherwise they would have started the infinite looping. After this change we're always sending the current local copy. This is still somewhat of a work-around but it's one that should reliably cover over the larger problems in the system. Eventually we have to clear out the data flow, remove local copies of our data, separate independent clients or sessions so they don't share buckets, and simplify the control flow to make things easier to debug.
belcherj
pushed a commit
that referenced
this pull request
Nov 18, 2019
Resolves #1690 Obviates session lock in: #1700, #1704, #1707, #1710, #1720 Resolves bug that uncovered signout/signin issue: #1664 Follows Simperium API change in: #1598, #1599, Simperium/node-simperium#61, Simperium/node-simperium#78 When we fixed some deep and underlying issues in `node-simperium` we started a chain of operations which had to adjust to that change. Remember that the core problem was an assumption that after we send out a change that we could wait until it came back. That assumption was wrong because changes from other remote clients could come in during that time period while we're waiting. Simperium/node-simperium#61 fixed that problem but because it was broken in the past we added some work-arounds in #706; these work-arounds complicated the flow of data in the corrected version so we had to remove them and change how we handle remote updates in the Simplnote side. A critical step in this flow is letting Simperium know what local changes we may have in the app buffer. We introduced this step in #1598 but never noticed a typo: ```js getState().selectedNoteId ``` That reference will always be wrong and so we'll never send the local contents of the note. It should have been... ```js getState().appState.selectedNoteId ``` The implication of this change is that we always told Simperium that there are no local changes and so it would grab the currenty copy in the bucket, which in our case is in shared `indexedDB` tables, and thus it would occasionally get updated copies of the bucket that were updated in another session. This would then appear as though we also made the same change we were receiving, and thus we would prepare our own patch to send out and start the cycle. Further we were only sending local updates in the case that we had the same note open as was updated in the remote change. However, since we don't update Simperium with notes that we're not editing we were leaving other notes behind by sending `null` for them. The outcome of this is that edits to notes that aren't selected would have a modification in the way they are applied, if otherwise they would have started the infinite looping. After this change we're always sending the current local copy. This is still somewhat of a work-around but it's one that should reliably cover over the larger problems in the system. Eventually we have to clear out the data flow, remove local copies of our data, separate independent clients or sessions so they don't share buckets, and simplify the control flow to make things easier to debug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #1579, #1598
See Simperium/node-simperium#78
See Simperium/node-simperium#61
When we receive an update from a remote client we have been listening
for it and adjusting our local app state to account for that change.
Unfortunately in cases where we also have local or pending changes we
have been erroneously transforming or rebasing the change before
applying it. The underlying
node-simperiumlibrary has alreadyperformd that transform, however, and when the client application
does that too it leads to double-writes and misapplied patches,
producing note corruption.
In this change we're stopping the rebase operation we have been
performing and that will remove this particular bug from the
application. This change does not solve all of the problem however
because we also have to make sure that the
node-simperiumlibrary isaware of our local state when it receives those updates.
Further work is taking place in Simperium/node-simperium#61 and #1598 to
close the loop on that but these changes are important enough on their
own to warrant a change.
This will close one bug while opening another but this is a dependent
part of the process in closing the broader issue.
Testing
This is going to be hard to test. It would be good to document the before/after
bugs and I will try to do that after I get back to where I have two computers on
which to test.
We're expecting bugs when we are crossing paths between local and remote
updates: that involves a couple of stages of "queues." One queue is text in the
editor component, another is the debounced-update from the same, another is
the local queue in
node-simperium, and the final (major) queue is the "wait"queue in
node-simperium.