[Bug Report] Server-Side Data Inconsistencies When Rapidly Creating Nodes in ShareDB about sharedb HOT 12 CLOSED

Thanks for the report! I'm not sure I fully understand the issue (I've not used the ot-tree type personally). Could you please provide a minimally-working reproduction, ideally with a failing test case.

from sharedb.

OK，I will provide more details tomorrow

from sharedb.

I posted a minimally-working project in https://github.com/TianBM/mini-ot

from sharedb.

Hi thanks for this. I see you've included a hand-rolled OT type in here. Is this your own type? Is the bug with the type or with ShareDB?

Also I don't fully understand this type. Could you please write a failing test case, with some assertion on the expected final state.

from sharedb.

I found it on GitHub and noticed that json1 has the same issue. I’m writing a unit test for ShareDB to help identify and fix the problem. I believe that ShareDB should ensure the operation (op) isn't modified, even if some OT types may change the op object.

from sharedb.

There are some json1 tests here already. Probably a good place to chuck a new test, and should hopefully give you a good starting point for a new test.

from sharedb.

yeah, I noticed that json0 has the clone op operation, which avoids using the same object for the push method and the apply method, so I would suggest that clone could be added before the sharedb's ottype.apply call to ensure that they don't affect each other

from sharedb.

I think this is a bug in ot-tree, but I also hope sharedb will consider my proposal to add the clone method before the ottype.apply call, to avoid the push method and the apply method affecting each other

from sharedb.

I agree cloning would be safest, but it does come with performance and maintenance penalties.

According to the docs:

apply(snapshot, op) -> snapshot': Apply an operation to a document snapshot. Returns the changed snapshot. For performance, old document must not be used after this function call, so apply may reuse and return the current snapshot object.

...the snapshot actively isn't guaranteed immutable, but no mention is made of the op, which I personally would read as: the type should not mutate the op, and it's a concern of the type to clone the op if it needs to mutate it for internal work.

Further, I think if the type mutates the op, I'm pretty sure it breaks this property of transform:

Transform must conform to Transform Property 1. That is, apply(apply(snapshot, op1), transform(op2, op1, 'left')) == apply(apply(snapshot, op2), transform(op1, op2, 'right'))

If apply() mutates the ops, I'm pretty sure you can't guarantee Transform Property 1, so the type is broken.

Also, there's points in ShareDB where we directly call type.apply(), and I don't think it should be a concern of ShareDB to have to clone before every call to this.

from sharedb.

I agree that ops shouldn't be mutable, but there are certain cases where, for example, the op object is added directly to the snapshot, in which case the ot-type may have all the behaviors met, but is affected by the subsequent apply method because it's not cloned

from sharedb.

That's true, but I'd still say that would probably fail Transform Property 1 I suspect. If a type is well-behaved, and knows that it is going to do this sort of thing, I'd say the onus is on the type to clone if it thinks it appropriate.

from sharedb.

I closed this issue, but I still think there is a problem: ot-type with all behaviors satisfied may cause wrong data because of the current sharedb writeup, I need some time to check out

from sharedb.