Sandbox "tried to invoke action '...' over a readonly node" when original tree contains reaction

[sisp]

I've discovered a problem when the sandbox copy of a node contains a reaction. When the reaction in the original node fires and triggers an action, the sandbox attempts to apply the changes made by the action to the sandbox copy, but it appears the patches are not applied inside allowWrite(() => { ... }):

[mobx] Encountered an uncaught exception that was thrown by a reaction or observer component, in: 'Reaction[Reaction@15]' MobxKeystoneError: tried to invoke action 'inc' over a readonly node

I believe the problem is caused by this onPatches listener:

https://github.com/xaviergonz/mobx-keystone/blob/7ff26354be93b5b5e3796e3a7c1773f3e03c2215/packages/lib/src/treeUtils/sandbox.ts#L79-L81

This is a test case (to be added to packages/lib/test/treeUtils/sandbox.test.ts) that reproduces the problem:

test("sandbox is patched when original tree with reaction changes", () => {
  @model("R")
  class R extends Model({ a: prop<A>(), n: prop<number>(0) }) {
    onInit() {
      autoDispose(
        reaction(
          () => this.a.b.value,
          () => {
            this.inc()
          }
        )
      )
    }

    @modelAction
    inc() {
      this.n++
    }
  }

  const r = new R({ a: new A({ b: new B({ value: 0 }) }) })
  const manager = sandbox(r)
  autoDispose(() => manager.dispose())

  r.a.b.setValue(10)

  expect(r.a.b.value).toBe(10)
  expect(r.n).toBe(1)

  manager.withSandbox(r, rcopy => {
    expect(rcopy.a.b.value).toBe(10)
    expect(rcopy.n).toBe(1)
    return false
  })
})

I suspect that the problem is related to the fact that a reaction is executed after an action has finished and the onPatches listener is wrapped as an action. In the code snippet I referenced above, this.allowWrite is called inside the onPatches listener, so if the reaction is executed after the listener has finished, it means the reaction is executed outside this.allowWrite. However, I tried removing

https://github.com/xaviergonz/mobx-keystone/blob/7ff26354be93b5b5e3796e3a7c1773f3e03c2215/packages/lib/src/patch/emitPatch.ts#L59-L61

to prevent the listener from becoming an action and the problem was still present. At this point, I'm not sure what exactly is causing this error.

[xaviergonz]

I'll try to take a look at it tomorrow.

[xaviergonz]

The error seems legit to me, it seems like the readonly mware is protecting you from a modification being made to the clone outside of withSandbox

 r.a.b.setValue(10) // changes value on original
// applies patches to clone (new value)
// runs reaction on original, n original is now 1
// applies patches to clone (new n)
// runs reaction on clone (because of new value), trying to set inc n to 2 (not how this would leave the clone and the original in different states if this action is actually performed)
// reaction crashes because the clone is being modified outside of a sandbox call

Maybe I'm missing something?

[sisp]

The problem is that this error occurs when I use, e.g., root refs which instantiate a reaction inside onInit. That's also the basis for the test case which only contains the essence. If this error is legit, then using the readonly middleware in this case may be incorrect.

[sisp]

Maybe the applyPatches call which patches the sandbox tree should never trigger reactions in the sandbox tree because if the tree contains reactions they are always triggered in the original tree and the sandbox tree should only be patched accordingly? Does this sound right to you?

[xaviergonz]

That sounds right, but I'm not sure how to pull that off automatically...

At least I guess the sandbox manager could offer a "isSandboxNode(node)" (maybe global to decouple it from a given manager?) function so the cloned node can avoid setting up the reaction if that function returns true.

Or maybe using a context set up on the root of the sandbox cloned node. (alternatively such global function could use a context as internal implementation)

[xaviergonz]

Although usually it is better to set up reactions on "onAttachedToRootStore" to avoid unwanted init code, but since sandbox clones replicate the root store state...

[sisp]

I believe I'm making some progress here. To test our hypothesis that an applyPatches call which patches the sandbox tree should never trigger reactions in the sandbox tree, I've monkey-patched MobX' reaction function and implemented a runWithoutReaction<R>(fn: () => R): R function to disable reactions for anything executed in fn, and it appears to work as expected. Test cases with reference resolution, idempotent reactions, and non-idempotent reactions all pass. Monkey-patching isn't nice though and I assume you wouldn't want this solution to be added to mobx-keystone, so I'll try to get runWithoutReaction into MobX. If things go well, #102 will become obsolete.

There is still one problem when applying patches generated from the original tree to the sandbox copy: The onPatches listener applies the patches to the sandbox copy using applyPatches inside this.allowWrite(() => { ... }) as the sandbox copy is readonly. However, for some reason the runWithoutReaction call seems to have no effect there, presumably because the reactions are triggered after the onPatches listener returns (because it is an action) and by that time runWithoutReaction has, of course, already returned, too. To address/discuss this issue, I've opened #105.

[xaviergonz]

Just wondering, wouldn't it be easier to set up some context on the sandbox root (for example a context that gives access to the sandbox manager that created such clone, or undefined if none), and let the user decide if a reaction should be performed or not when sandboxed?

e.g.

// context and methods offered by library
function getSandboxManager(node: object) {
  return sandboxManagerCtx.get(node)
}

function isSandboxed(node: object) {
  return !!getSandboxManager(node)
}

// user code
@model("R")
  class R extends Model({ a: prop<A>(), n: prop<number>(0) }) {
    onInit() {
      // note we don't check isSandboxed here because context wouldn't yet be set / inherited
      autoDispose(
        reaction(
          () => this.a.b.value,
          () => {
            if (!isSandboxed(this)) {
              this.inc()
            }
          }
        )
      )
    }

    @modelAction
    inc() {
      this.n++
    }
  }

My point is that there might be legit cases when you may want to run reactions on sandboxed nodes.

[sisp]

It would be easier for sure. But what happens if there are reactions that a user has no control over, e.g. reference resolution (which is idempotent, so actually not a problem)? I still believe that it is wrong in general to execute reactions upon patching the sandbox copy, but I might be overseeing a valid case. I also realized that there is no problem when reactions are idempotent (unlike inc() which is non-idempotent). Also, reactions that make HTTP requests are likely not desired to happen in the sandbox copy which would be prevented by runWithoutReaction, but it is likely that a user has control over such reactions and your suggested solution would work, too.

Since the current state of the sandbox util is not entirely correct, perhaps it makes sense to prioritize the next steps:

1. Fix the original problem of this issue

   [mobx] Encountered an uncaught exception that was thrown by a reaction or observer component, in: 'Reaction[Reaction@15]' MobxKeystoneError: tried to invoke action 'inc' over a readonly node

   which I believe is exclusively related to #105 and https://github.com/xaviergonz/mobx-keystone/issues/96#issuecomment-573736538. I may need your help on this because I'm not sure why the reaction is executed after the onPatches listener has returned and whether there is anything wrong with changing this behavior.

2. Set up a sandbox context on the sandbox root as you suggested to let a user decide if some piece of code should be executed or not depending on whether it's the sandbox copy or the original tree.

3. OPTIONAL: Discuss runWithoutReaction with the MobX community and, if accepted, use it to patch the sandbox copy without triggering reactions. Perhaps we could make this an opt-out feature in case someone has a valid case in which it is wrong to not execute reactions.

What do you think?

[sisp]

Your suggested solution works well for me and I haven't experienced this problem for quite some time, so I'll close this issue.