fix: concurrency control in persistence layer

[lei9444]

Description

Now in file persistence use notify to sync with server. Sometimes may run a series of async operations.
For move will call following actions: create dialog(will create lg and lu) -> update lu -> update lg -> update dialog -> update lu -> update lg, all these run in parallel, So the update lg may be returned earlier than create lg and the data is overwritten.

update:

1. Add a queue to store the changed file info.
2. For persistence layer: add a tag to protect current concurrent events.
3. Merge the change for the same files in the queue. and we can run multiple tasks concurrently for different files

Task Item

refs #2718

Screenshots

[boydc2014]

Glad to see this change, please added a little bit more on the design. It's probably OK to be simply just make every change just sequential.

The good news is this is now only a problem in persistence layer, whichever strategy we toke is a problem of persistence, which is great.

[yeze322]

A js-styled solution

class FilePersistence {
  private tasks = []
  private isRunning = false;

  public persist(file) {
    this.tasks.push(file);
    this.runAll();
  }

  private pushFile(file) {
    this.tasks.push(file);
  }
  private popFile() {
    this.tasks.shift(file);
  }

  private async runAll() {
    if (this.isRunning) return;
    this.isRunning= true;
    while(tasks.length) {
      const file = this.popFile();
      await save(file);
    }
    this.isRunning= false;
  }
}

[yeze322]

A more advanced solution

class FilePersistence {
  private fileQueue = new FileQueue();
  private isRunning = false;

  public persist(...files) {
    this.fileQueue.push(...files);
    this.runAll();
  }

  private async runAll() {
    if (this.isRunning) return;
    this.isRunning= true;
    while(!this.fileQueue.isEmpty()) {
      const file = this.fileQueue.pop();
      await save(file);
    }
    this.isRunning= false;
  }
}

Define a file queue.

class FileQueue {
  private tasks = [];
  private push(...files) {
    // foreach file in files
    const fileIndex = this.tasks.find(x => x.id === file.id);
    if (fileIndex > -1) {
      // merge an existed file
      this.tasks[fileIndex] = merge(this.tasks[fileIndex], file);
    }
    this.tasks.push(file);
  }
  private pop() {
    return this.tasks.shift();
  }
  private popList() {
    // pop all unique file ids
    return [...];
  }
}

[yeze322]

validated on my local PC, can fix the behavior of 'Move'

[boydc2014]

Thanks @yeze322 , good advices, we need to make sure the granularity is not too small, which means we should allow concurrency in between dialogs, which is reasonable of this stage.

But i'm not a fan of "FileQueue", a generic queue is good enough, there were plenty of implementation of threadpool is the old days using the model of queue+workers.

It would be better for queue is just queue, only work as a storage as your message, but not aware of any internal data shape.

Make sense?

[boydc2014]

Perhaps let me be concrete about my suggestions @lei9444 @yeze322 , the princinple is decouple how to store the data with how you process the data.

1. I will suggest explicit maintain a map { key => queue } , key is a "generic tasking sharding key" which in your case happens to be file.id, instead of creating new types of storage only for your case
2. I will suggest keep the original change diff intact without merging those, when you save to store, i mean you can merge them when you process them. This is not a big deal in this case, but a good pattern.

[boydc2014]

see above

[yeze322]

About Undo/Redo, I had an idea and evaluated it with @lei9444, it's the solution with least effort.

Fix undo/redo by 3 steps:

1. Force all dialog edit events fired through the useDialogEditApi, make sure every change tracked.
2. Let the useDialogEditApi create a snapshot when a series of action finished (e.g. 'Move', 'Copy-Paste')
3. In the snapshot stack, a snapshot contains all parts of dialog data, undo / redo simply picks a snapshot to replace Shell store; then Shell store will be persisted to files.
   

[Shell -> Persistence Layer] - We may need a json diff module here, whenever the dialog data in store changes, dumps changes to files. cc @zhixzhan @boydc2014 (Or we simply perform a whole project rewrite?)

[cwhitten]

Guys I'm referring to this comment here: #2718 (comment)

It is not addressed with this change like @yeze322 said it would be. That is what I'm inquiring about. I understand the Undo/Redo behavior is not a component of this work.

[boydc2014]

Guys I'm referring to this comment here: #2718 (comment)

It is not addressed with this change like @yeze322 said it would be. That is what I'm inquiring about. I understand the Undo/Redo behavior is not a component of this work.

I expect this PR is a dependency for Ze's PR on #2718. This is a issue reported by Ze and Leilei think this is a general issue, so this PR is against master. Next step is for @yeze322 to merge master on #2718 after we take this in master, and complete any fixes there, make sense? @cwhitten

[yeze322]

This PR should make LU/LG copy paste move work, I've validated on several cases

• LG/LU copy paste -> paste to the same trigger, paste to another trigger / dialog
• LG/LU move -> move to a new dialog

Are there some senarios I didn't covered? @cwhitten

[lei9444]

Hi, @cwhitten @boydc2014 The throttle wait time has been set to 0, and I test move more then two actions works now. But we need to reopen the lg editor perf issue now. And I will try to fix the issue.

[boydc2014]

Hi, @cwhitten @boydc2014 The throttle wait time has been set to 0, and I test move more then two actions works now. But we need to reopen the lg editor perf issue now. And I will try to fix the issue.

Just remove the throttling is cleaner than setting to 0. Rethink the perf issue with a follow up on parsing.

[lei9444]

Hi, @cwhitten @boydc2014 The throttle wait time has been set to 0, and I test move more then two actions works now. But we need to reopen the lg editor perf issue now. And I will try to fix the issue.

Just remove the throttling is cleaner than setting to 0. Rethink the perf issue with a follow up on parsing.

Now set wait time to 0 can make it work. We need to remove it, but I think I should do some investigate about this. Some code related maybe need to be cleaned too. I want to remove it in lg editor perf PR.

[lei9444]

Hi @cwhitten @boydc2014 #2823 will fix the throttle issue in move and copy, and I will have a new PR to fix the performance issue about lg and lu editors.

[cwhitten]

sounds good @lei9444