Update automerge, re-add save decision

[gterzian]

This re-adds the decision to do a full vs incremental saves, and does so using automerge 0.5.0.

This is a separate branch because it appears bumping automerge breaks the integration, so I've left the other branch alone for now.

[issackelly]

I don't think that I understand the interplay between "patches since last save" and compaction.

IIUC, compaction saves the entire document as a snapshot and destroys all previous snapshots and incrementals, so this would only occur if there were > 10 patches before an incremental?

Shouldn't it only be reset after compacting?

[gterzian]

Shouldn't it only be reset after compacting?

Yes you're right, I think this should be moved to the conditional branch that does compaction.

Also, we may want to add logic that does a compaction on the first save? Or can the first one be incremental?

[issackelly]

I have a branch that allows compaction on the first save, but first being incremental isn't a big deal as long as we don't throw it away. I'll try to separate out those commits as separate PRs

[issackelly]

Yes you're right, I think this should be moved to the conditional branch that does compaction.

Yeah I agree, thanks.

[issackelly]

Here it is: faa58e9

I have a ~junk PRs that shouldn't be merged here, but it's where I'm collecting all the changes I need to get this into production, we can make tiny PRs as appropriate.

[issackelly]

Here's a commit that addresses the patches_since 962b38c

[issackelly]

I think this introduces a deadlock in note_changes -- when it is set to true the tests pass, but with this implementation, they hang.

What about a thread local that simply counts the number of times that save_document was run?

[gterzian]

I think this introduces a deadlock in note_changes -- when it is set to true the tests pass, but with this implementation, they hang.

What about a thread local that simply counts the number of times that save_document was run?

That test was badly written :)

I have pushed a fix to your compaction branch. @issackelly

Should we move the discussion over to #50 and close this one?

UPDATE: it seems to still hang intermittently, looking into it...

[gterzian]

@issackelly

Ok, I have pushed a fix to your branch(it still contains prints and so on, sorry about that).

This was the race condition:

In the handling of the RepoEvent::DocChange, we would only handle the event if note_changes returned true.

This would be false if the backend had saved the document after the local change had been committed, but before the RepoEvent::DocChange had been received. This would happen if the save had been triggered by an incoming sync message(could have happened also due to a previous doc change event sent out by another handle to the same doc).

In other words, when doing the save in response to the incoming sync message, the current state of the document already included the last local change, so the subsequent RepoEvent::DocChange would be ignored.

Ignoring the subsequent even resulted in not sending out any sync messages for that latest change(which had been saved already).

---

I've pushed a change to your branch that in such a case only skips the saving and the waking up of change observers, not the sending out of sync messages.

In general there is a complication around changes to the local document, which happen via the lock around the shared doc, and event handling around those changes. Another example was waking up handles on changed.

I think the current fix may have unintended consequences with regards to waking up change observers(although I cannot think of one as of now, and they would have been skipped before also in such a case). It may make sense to at some point separate out the last_heads concept between one used for saving only, and one used for other things(sending out sync, waking up on changed). Or perhaps a simple count like you suggested above could be used. The important part would be separating the data used for the various decisions, if they can be separated.

Tracked at #51

[issackelly]

Thanks! Good catch and fix on this. I left a note about the read_lock on note_changes in #51 but we can continue there.

[alexjg]

Superceded by #58