yorkie-js-sdk icon indicating copy to clipboard operation
yorkie-js-sdk copied to clipboard

Trigger incorrect TreeChange during concurrent tree.style editing

Open chacha912 opened this issue 1 year ago • 1 comments
trafficstars

What happened:

While concurrently editing a document in tree style, the TreeChange is sent incorrectly.

  1. Initial State: <doc><p italic="true">hello</p></doc>
  2. ClientA and ClientB are editing concurrently:
    • Client A: t.style(0, 1, { bold: 'true' })
    • Client B: t.style(0, 1, { bold: 'false' })
  3. Final Document State: <doc><p bold="false" italic="true">hello</p></doc>

Though the final document state converges correctly, the treeChange received by users through doc.subscribe is as follows: As a result, in docB, the last bold: true is applied, causing divergence in the document displayed in the editor.

// docA 
[
  {
    type: 'tree-style',
    from: 0,
    to: 1,
    value: { bold: 'true' },
  } as any,
  {
    type: 'tree-style',
    from: 0,
    to: 1,
    value: { bold: 'false' },
  } as any,
],

// docB 
[
  {
    type: 'tree-style',
    from: 0,
    to: 1,
    value: { bold: 'false' },
  } as any,
  {
    type: 'tree-style',
    from: 0,
    to: 1,
    value: { bold: 'true' }, 
  } as any,
],

What you expected to happen:

If there are no document changes, a change should not be triggered.

How to reproduce it (as minimally and precisely as possible):

To reproduce the issue, you can run the following test code:

it.only('Concurrent style and style', async function ({ task }) {
  await withTwoClientsAndDocuments<{ t: Tree }>(async (c1, d1, c2, d2) => {
    d1.update((root) => {
      root.t = new Tree({
        type: 'doc',
        children: [
          {
            type: 'p',
            children: [
              {
                type: 'text',
                value: 'hello',
              },
            ],
            attributes: { italic: 'true' },
          },
        ],
      });
    });
    await c1.sync();
    await c2.sync();
    assert.equal(d1.getRoot().t.toXML(), d2.getRoot().t.toXML());
    assert.equal(
      d1.getRoot().t.toXML(),
      /*html*/ `<doc><p italic="true">hello</p></doc>`,
    );

    const [ops1, ops2] = subscribeDocs(d1, d2);

    d1.update((r) => r.t.style(0, 1, { bold: 'true' }));
    d2.update((r) => r.t.style(0, 1, { bold: 'false' }));
    await c1.sync();
    await c2.sync();
    await c1.sync();
    assert.equal(d1.getRoot().t.toXML(), d2.getRoot().t.toXML());
    assert.equal(
      d1.getRoot().t.toXML(),
      /*html*/ `<doc><p bold="false" italic="true">hello</p></doc>`,
    );

    assert.deepEqual(
      ops1.map((it) => {
        return { type: it.type, from: it.from, to: it.to, value: it.value };
      }),
      [
        {
          type: 'tree-style',
          from: 0,
          to: 1,
          value: { bold: 'true' },
        } as any,
        {
          type: 'tree-style',
          from: 0,
          to: 1,
          value: { bold: 'false' },
        } as any,
      ],
    );

    assert.deepEqual(
      ops2.map((it) => {
        return { type: it.type, from: it.from, to: it.to, value: it.value };
      }),
      [
        {
          type: 'tree-style',
          from: 0,
          to: 1,
          value: { bold: 'false' },
        } as any,
        // {
        //   type: 'tree-style',
        //   from: 0,
        //   to: 1,
        //   value: { bold: 'true' },
        // } as any,
      ],
    );
  }, task.name);
});

Anything else we need to know?: Related: https://github.com/yorkie-team/yorkie-android-sdk/pull/159

Environment:

  • Operating system:
  • Browser and version:
  • Yorkie version (use yorkie version):
  • Yorkie JS SDK version:

chacha912 avatar Feb 27 '24 05:02 chacha912

Unlike text style, which stores nodes requiring style application in toBeStyleds using latestCreatedAt and adds changes only for those nodes, the tree style implementation currently lacks latestCreatedAt.

  • https://github.com/yorkie-team/yorkie-js-sdk/blob/8b6ae9788f1b33c52930aae885319a4841eb84b1/src/document/crdt/text.ts#L266-L292
  • https://github.com/yorkie-team/yorkie/pull/795

~~Another approach might involve comparing previous attribute values and omitting changes if there are no differences.~~

chacha912 avatar Feb 27 '24 06:02 chacha912

~Dup of https://github.com/yorkie-team/yorkie/issues/788.~

hackerwins avatar May 09 '24 00:05 hackerwins