dominictarr / crdt

Commutative Replicated Data Types for easy collaborative/distributed systems.
MIT License
836 stars 43 forks source link

items dissappear from sequence when moved #34

Closed nrw closed 10 years ago

nrw commented 10 years ago

I'm picking on crdt, but the problem could be in level-scuttlebutt or level-js. I made an example here https://github.com/nrw/seq-example.

summary: open doc. fill sequence. move items around in the sequence. close doc. reopen. any moved item is missing from the sequence.

Is my usage wrong or is something broken?

nrw commented 10 years ago

Update: the problem persists when level-js is omitted from the example.

dominictarr commented 10 years ago

a manual test is not good, if you have found a bug you need to make an automatic test. can you make this test do the wait and then check thing automatically?

this may be a sync race condition, the after overwrites the push({id: 'a'})... it probably works if you do this asynchronously (because no one has complained about this before), but this should work, thierefore what is needed is a testcase.

nrw commented 10 years ago

I'll try to find a way to make this test automatic.

nrw commented 10 years ago

I just pushed an automatic test case: https://github.com/nrw/seq-example

assertion should pass. it fails.

dominictarr commented 10 years ago

Hmm, can you make this work just in one process without using child_process? then it will be easyiest to debug.

nrw commented 10 years ago

This is my attempt. In this one, the sequences still don't match, but they don't match in a different way. https://github.com/nrw/seq-example/blob/master/single-process.js

The first sequence is as expected. the second is totally empty.

nrw commented 10 years ago

I just pushed an update. This test case now reproduces the original symptoms in a single process: https://github.com/nrw/seq-example/blob/master/single-process.js

dominictarr commented 10 years ago

okay, digging into this - it's probably actually a level-scuttlebutt issue... maybe because of nextTicks. I've upated level-scuttlebutt to test with the latest deps, and it's failing one test. this may be related...

nrw commented 10 years ago

i thought it might be a level-scuttlebutt issue. thanks for digging in!

dominictarr commented 10 years ago

Aha... I have found the problem!

It is about the way that crdt tries to cleanup old updates - it creates one update that adds a into the sequence - and then the next update moves it after b, yet it was deleting the first update.

by filtering the deletes out I got it to work - but the solution is getting crdt to emit the correct _remove events

dominictarr commented 10 years ago

This is fixed in 3.6.2! also, you should upgrade to level-scuttlebutt@5.0.8 which has your script added as a test. (with the small change to use toJSON instead of asArray)

nrw commented 10 years ago

Hooray! Thanks!