fix(Project): prevent potential data corruption in saveToCloud and saveToLocalCache

[aofei]

Fixes #761

[nighca]

这次解决的问题方便通过构造测试用例复现吗，方便的话加下用例？

[aofei]

如果要构造测试用例来复现，那可能得 mock Disposable，因为 saveToCloud/saveToLocalCache 里直接依赖了它的 isDisposed。我在想有没有必要，因为问题的根源是

  constructor() {
    super()
    const reactiveThis = reactive(this) as this
    this.history = new History(reactiveThis)
    this.zorder = []
    this.stage = new Stage()
    this.sprites = []
    this.sounds = []
    this.addDisposer(() => {
      this.sprites.splice(0).forEach((s) => s.dispose())
      this.sounds.splice(0).forEach((s) => s.dispose())
    })
    return reactiveThis
  }

这里面的 sprites 和 sounds 在 dispose 时被清空了，而自动保存在 dispose 后才执行。

我们应该只用在测试中确保 isDisposed 为 true 时，saveToCloud 或者 saveToLocalCache 会抛出异常就行了吧？

[nighca]

我们应该只用在测试中确保 isDisposed 为 true 时，saveToCloud 或者 saveToLocalCache 会抛出异常就行了吧？

嗯可以的

[nighca]

确实现在预期几乎所有的 project 内容的变更都会被 history 的 mutex 包起来，不过这不是这个 mutex 的原意，它本来的目的只是做 history 操作间的互斥，针对的是并发的多个 doAction（或 undo / redo）调用互相穿插导致 history 记录版本的混乱

如果这里我们用它，是把它的定位当成“project 内容变更的锁”的话，可能把定义挪到 class Project 更合适（从依赖方向上说，目前也主要是 history 实现去依赖 project 的能力，而不是 project 实现去依赖 history 的能力）

另外还有个衍生的问题是，如果 saveToCloud 中的 export 调用加这个互斥逻辑是合适的话，那么其他地方的 export 调用是不是也应该加？比如 exportGbpFile 中、saveToLocalCache 中。我觉得可能可以把这个 mutex 逻辑直接加到 export 逻辑里（只是那样的话我们会需要把 export 改为异步方法）

[aofei]

另外还有个衍生的问题是，如果 saveToCloud 中的 export 调用加这个互斥逻辑是合适的话，那么其他地方的 export 调用是不是也应该加？比如 exportGbpFile 中、saveToLocalCache 中。我觉得可能可以把这个 mutex 逻辑直接加到 export 逻辑里（只是那样的话我们会需要把 export 改为异步方法）

如果把 mutex 逻辑做进 export，除了像你说的需要把 export 改为异步方法可能会带来些麻烦外，比较大的一个变化是我们默认了导出操作是可能导出中间态数据的，不再交由调用方根据其所处的上下文去判断，这样会是预期的吗？另外，这样我可能就会纠结 exportGameFiles 是不是也应该有 mutex 逻辑呢（虽然它相对 export 更 low-level 些）？



目前的写法是认为，只有在自动机制中才可能存在导出中间态数据的情况，因为只有它可能在用户操作 history 变更时执行。这也是为什么我只针对 saveToCloud 和 saveToLocalCache 做了处理。不过我刚注意到 Export project file 功能里面并没有阻塞 UI 交互，虽然 exportGbpFile 速度很快，但也确实存在导出期间由于 UI 交互导致 history 变更，进而使用了中间态数据的可能，我忽略了这点。

我可能比较倾向于让 export/exportGameFile 的调用方根据上下文来决定是否需要上锁。比如 ProjectCreateModal.handleSubmit 里对 export 的使用，还有 History.saveCurrentState 里对 exportGameFile 的使用，都是不需要上锁的。

但是，如果认为 export 跟 saveToCloud 它们一样属于是比较“high-level”的方法，那直接把 mutex 逻辑做进去就没啥毛病了。相应的我也不会再纠结 exportGameFiles 是不是也应该有 mutex 逻辑，因为它是“low-level”的。

以上是我做的时候的想法。



至于是否把 mutex 逻辑直接做进 export，我都 ok。单从使用负担上来讲，像你说的那样直接做进 export 的话使用起来可能会更轻松些。

确实现在预期几乎所有的 project 内容的变更都会被 history 的 mutex 包起来，不过这不是这个 mutex 的原意，它本来的目的只是做 history 操作间的互斥，针对的是并发的多个 doAction（或 undo / redo）调用互相穿插导致 history 记录版本的混乱

如果这里我们用它，是把它的定位当成“project 内容变更的锁”的话，可能把定义挪到 class Project 更合适（从依赖方向上说，目前也主要是 history 实现去依赖 project 的能力，而不是 project 实现去依赖 history 的能力）

其实我是把它的定位当成了“project history 变更的锁”，这里的使用更像是 History.lock 和 History.unlock。

我们可能不需要引入“做任何内容变更前都需要考虑锁”，而是维持现有的“任何内容变更都应该发生在 History 中以产生 history”。所以如果 Project 里要有个锁，命名上我可能也会更期望它是 historyMutex 而不是 mutex，后者会让人纠结该什么时候使用它。

[nighca]

比较大的一个变化是我们默认了导出操作是可能导出中间态数据的，不再交由调用方根据其所处的上下文去判断，这样会是预期的吗？

是的，这里是一个行为上的调整

如果不是因为实现上的不得已，其实我们是更希望“中间态”根本不存在的；如果在不影响交互体验的前提下，我们能做到包括 load 在内的任何操作都立即完成、与其它任何代码互斥，那样我们应该会更容易实现需求，且业务上能力不会由此受限；从这个前提出发，我认为“导出中间态数据”应该不是一个需要被满足的诉求，“常规”的业务诉求应该都可以通过导出某次变更完成后的状态来满足。

而由每一个 export 的调用方去做互斥的话，一方面负担会重一些，另外一方面也容易让外部接触到细节，毕竟 export 是预期可以由 class Project 外部去调用，也确实有外部会直接调用的。

另外，这样我可能就会纠结 exportGameFiles 是不是也应该有 mutex 逻辑呢（虽然它相对 export 更 low-level 些）？

这会是个问题，我建议把 exportGameFiles 做成 private，然后 history 也去依赖 export/load 而不是 exportGameFiles/loadGameFiles，这样就不用纠结了——export/load 是收口子的地方；当然这里会有一些细节要处理，比如以 history 目前的定位，当它调用 project.load 的时候，是只希望更新其中的 game files 的，那么 load 可能需要支持可选的 metadata 参数。

比如 ProjectCreateModal.handleSubmit 里对 export 的使用，还有 History.saveCurrentState 里对 exportGameFile 的使用，都是不需要上锁的。

ProjectCreateModal.handleSubmit 里的 export 不需要考虑互斥是因为它简单，如果构造 & 初始化的逻辑复杂一些（比如通过 load 一个远程的模板项目完成初始化），那么应该是需要上锁的（或者类似地，由外部逻辑去明确地等待 load 完成）

History.saveCurrentState 里对 exportGameFile 的使用不需要上锁是因为 history 自己在业务上有锁了；如果 export/load 跟 history 各自维护自己的 mutex，后者的粒度恰好是要比前者更粗的，因此可以不去考虑前者

这俩情况都是不用锁目前不会有问题，但是用也合理

其实我是把它的定位当成了“project history 变更的锁”

是指把“project 数据变更”跟“history 记录变更”合成一个概念，然后这个锁的主题就是针对这个概念的？我觉得可以的，虽然我有点担心这俩概念后面会分化，不过现在看起来还好。mutex 确实不是个好名字，它没有说明它的定位/主题，叫 historyMutex 我没问题

[nighca]

噢不过好像我上面建议的

history 也走 export，且 export 加锁

跟

把“project 数据变更”跟“history 记录变更”合成一个概念

是冲突的..那样好像会死锁..

这么看如果共用一个锁，还是得先保留 exportGameFiles 作为 low-level API 供 history 用

[aofei]

噢不过好像我上面建议的

history 也走 export，且 export 加锁

跟

把“project 数据变更”跟“history 记录变更”合成一个概念

是冲突的..那样好像会死锁..

这么看如果共用一个锁，还是得先保留 exportGameFiles 作为 low-level API 供 history 用

嗯应该只能保留 public exportGameFiles。

刚想通了一个点。现在 History 的定位是，依赖 Project，并为 Project 的 game files 提供 undo/redo 能力扩展，以供给其持有方去使用，而不是在其内部使用。所以 History 所依赖的东西（比如锁）放进 Project 是没啥毛病的。

如果后面 History 的定位变成通用，不再局限于 Project 而是可以给任意 files 场景提供 undo/redo 能力，那 History 的构造函数就可以改成接收 loader&exporter 了，那样 loadGameFiles&exportGameFiles 也就可以很自然地私有化了。

[nighca]

是的，目前 history 定位上是只用于 project 的

如果后面 History 的定位变成通用，不再局限于 Project 而是可以给任意 files 场景提供 undo/redo 能力，那 History 的构造函数就可以改成接收 loader&exporter 了，那样 loadGameFiles&exportGameFiles 也就可以很自然地私有化了。

没毛病

[qiniu-ci]

The PR environment is ready, please check the PR environment

[Attention]: This environment will be automatically cleaned up after a certain period of time., please make sure to test it in time. If you have any questions, please contact the builder team.