Infinite recursion with arrays

[theSherwood]

@fabiospampinato

I've been running into a lot of difficult-to-debug issues around infinite loops involving arrays and circular references. Here's a sandbox with a sample of what I'm talking about:

https://codesandbox.io/s/voby-demo-store-counter-forked-r225g4?file=/src/index.tsx
(click the + a couple times and the app will hang)

This has been a bit difficult in my app because Solid handles these circular references in arrays just fine and I have data of a similar shape cropping up in numerous places in my app. So this looks to me like a bug, but what do you think? Are these cases something that voby/oby should handle? Or does there just need to be a big warning sign in the readme?

[theSherwood]

From the limited amount I've been able to figure out, it seems like the store-handling code may be recursively wrapping arrays in proxies on dereference. Honestly, I don't know what's happening.

[fabiospampinato]

Do you have a standalone repro outside of Stackblitz, preferably something that I can execute in Node? It you write state.nested.arr; instead of console.log(state.nested.arr) then the issue disappears, but console.log as far as I know is special in the browser in that it doesn't trigger proxy traps, so like whether you log something or not is kinda undetectable in user code, except by timing things, but Stackblitz is not a browser, and that console.log changes something as far as the app is concerned, so presumably the one used in Stackblitz can't handle circular references or something?

[theSherwood]

I don't currently have a standalone repro outside of codesandbox at the moment, but I am experiencing similar issues in my app, which is not in codesandbox. I've updated the codesandbox to show that its console.log can handle circular references without the oby proxy.

But if we unwrap the store and then try to log it, it freezes in the same way. I think this is because the unwrap isn't deep, it doesn't go down into the array and unwrap the proxy in there. So when it is logged out, we are left with the same problem we had originally.

[fabiospampinato]

console.log-ing shouldn't cause anything to break, so fundamentally that demo showcases a bug in Stackblitz really. If you are experiencing similar issues in your app without a repro I can't begin to look into this. unwrap not being deep is by design, if it was deep it wouldn't be constant-time anymore, and I don't see why that function shouldn't be constant-time. Note though how when stores are used normally, i.e. when there aren't extraneous Proxy instances in the store, and wrapped stores aren't assigned to properties of unwrapped stores, then unwrapping like this effectively gives a deeply unwrapped object, with no proxy instances anywhere.

Basically I've no idea what the actual issue is here, if you can make a repro that I can run in Node I'll look into it 👍

[theSherwood]

Okay. That's understandable. Something about codesandbox console.log is interacting badly with oby proxies. However, it's worth noting that this does not happen with Solid's proxies.

Digging into the issue locally, I've tracked my problem down to something about the way the serialization code in the UtilityScript of the playwright testing framework interacts with oby proxies. I haven't been able to figure out exactly what or why. I'm guessing that the codesandbox console.log must be doing a similar kind of serialization for its custom-rendered log in the iframe. But it's worth noting that I also didn't have these playwright issues with Solid's proxies.

So that appears to be two cases of widely-used serialization code (codesandbox console.log, playwright) that are causing infinite loops with oby proxies. My guess would be that this is going to come up again with other (possibly widely-used) serialization code.

[theSherwood]

Here's a repro repo https://github.com/theSherwood/voby-store-repro

Notice that the issue happens with proxies and unwrapped proxies, but not with the circular object that was never proxified.

Edit: Also notice that the limits which are put in place to prevent infinite recursion are somehow never triggered but the page will hang after clicking + regardless.

[fabiospampinato]

In general it's not really possible to guard against buggy code though.

I see the issue now, basically when one does this:

state.nested.arr = [...state.nested.arr, state.nested];

Or just this, presumably:

state.nested.arr = [state.nested];

Then what happens is that internally a proxy gets set on the unproxied object, which causes unwrap to not unwrap that, because our unwrap is constant time (in contrast to Solid's which goes deep), which causes other stuff down the line to fail because of the proxy references.

There are potentially many ways to try to address this, none ideal:

1. store.unwrap could unwrap deeply: I think that's kind of a bug in Solid actually, because now unwrapping is always slow, even when you don't need it, and you can't implement the constant-time unwrapping on top of the deep unwrap function, while you can implement the deep unwrap function on top of the constant-time one.
2. potentially store.unwrap(foo, { deep: true }) could be added? It feels kinda wrong though, because it kinda pushes you to write slow code, and it requires some understanding of the internals of the proxy to understand when to use that anyway, which one could use to not write the problematic code in the first place.
3. manually unwrapping stuff before setting it: if instead of [...state.nested.arr, state.nested] one writes [...state.nested.arr, state.nested].map(store.unwrap) then the problem disappears, because a proxy is no longer attached to the unproxied object.
4. potentially the store itself internally could do the unproxying, but it would mean deeply unproxying the set value, every set value, slowing down every set operation, basically.

I'm not sure what the best way to address this would be, I need to think about it 🤔

What do you think should happen?

[theSherwood]

In general it's not really possible to guard against buggy code though.

Yeah, that's true, but I don't think that's what's going on in this case though. That serialization code isn't buggy for circular objects. Maybe you're saying that the bug is to apply that serialization code to the proxies. But my impression is that if a proxy can't be treated more or less like the object it's proxying, that's surprising. I would expect instances of classes to not behave as plain objects, but I sort of expect everything that works on a plain object to also work on a proxy. If that's not the case, I think there should be warning signs.

potentially the store itself internally could do the unproxying, but it would mean deeply unproxying the set value, every set value, slowing down every set operation, basically.

I thought it already unproxied set values, just not with array items? Have I got that wrong?

potentially store.unwrap(foo, { deep: true }) could be added?

That was my first thought, But then I'm not sure how that would work. You'd have to modify or make copies of arrays when doing an unwrap (if it has proxy items) and that seems really surprising. But it seems like you'd have that same issue with any deep unwrap, which seems like a problem.

thing = Proxy({
  foo: 3,
  arr: [Proxy({bar: 3})]
})

If you try to deeply unwrap thing in the pseudocode above, what happens to thing.arr? Seems like you either have to modify it in place or have to make deep copies to unwrapping when there are nested arrays. Both of those options seem bad.

manually unwrapping stuff before setting it

This is simplest but means losing reactivity when reaching through a nested array, which seems less than ideal.

What do you think should happen?

I'm not sure. It's a tricky problem. My understanding is that if array methods were proxied, then unwrapping could still be constant time without losing the nested reactivity. Is that correct?

[fabiospampinato]

I sort of expect everything that works on a plain object to also work on a proxy. If that's not the case, I think there should be warning signs.

I guess that's a little surprising, and there should be warning signs, but I guess they should be on MDN or something? Like a proxied object is not triple-equal to the unproxied object. structuredClone throws for proxies. postMessage also throws. It's not Oby's place to document this stuff probably, though some warnings can be added to the readme if they are useful of course.

I thought it already unproxied set values, just not with array items? Have I got that wrong?

Yes it already unproxies set values, but not deeply. In your code the proxy is found "deep" inside the array, so that doesn't get unproxied.

That was my first thought, But then I'm not sure how that would work. You'd have to modify or make copies of arrays when doing an unwrap (if it has proxy items) and that seems really surprising. But it seems like you'd have that same issue with any deep unwrap, which seems like a problem.

Yeah that's another problem with deep unwrapping, especially if done by default. You deep unwrap twice and the two objects don't match one another. That's how Solid works by the way.

manually unwrapping stuff before setting it

This is simplest but means losing reactivity when reaching through a nested array, which seems less than ideal.

Losing reactivity how? This option should JustWork™️, it "just" requires some understanding of this problem and manual error-prone code that unwraps values being set.

I'm not sure. It's a tricky problem. My understanding is that if array methods were proxied, then unwrapping could still be constant time without losing the nested reactivity. Is that correct?

I'm not sure what you mean, it seems orthogonal to the problem at hand 🤔 array methods are already proxied.

[theSherwood]

Losing reactivity how? This option should JustWork™️, it "just" requires some understanding of this problem and manual error-prone code that unwraps values being set.

This is surprising to me (but in a good way). I just tested this out and it works fine. I don't know why I thought it didn't.

Yes it already unproxies set values, but not deeply.

Gotcha. I think that you are right that it shouldn't unproxy deeply by default.

I think I am seeing where my surprise is coming from. With Solid proxying/unproxying deeply, if I set an array on a proxy, the elements are proxied/unproxied appropriately automatically. Oby does not do that. But if you have a proxy with an array and push something onto that array, it is proxied/unproxied appropriately. So there's a difference in Oby with proxy handling depending on whether you replace the array with a new array or whether you modify the array in place. This a pretty surprising difference but makes sense given the assumptions of the library. It's just going to be a potentially sharp edge if you're coming from Solid. Maybe a "Migrating from Solid" page would be helpful here.

Part of me wonders whether array items should be proxied/unproxied by default if you set an array in a proxy. So not "deep" per se, but 1 layer in. That doesn't feel great as a solution but it does cover what may be a common case.

Given the severity of the bug (I lost several hours to this), something ought to be done. Maybe a "migrating from Solid" page is enough. Maybe proxying/unproxying array contents if an array is set causes other surprises later. I really don't know.

Tangential to this, the different philosophies around stores between oby and solid lead me to expect oby to perform worse with deep stores that get accessed repeatedly because the nested objects have to be reproxied lazily at every access. Is this correct or do you use a WeakMap to cache proxies?

Edit: for clarity

[fabiospampinato]

Re something ought to be done: I think maybe what should happen is that a warning for this should added to the readme, explaining how setting a value that has a proxy inside it can lead to issues with unwrapping, and potentially also a {deep: true} version of unwrap should be provided, to unblock people stumbling into this right away, making it their choice if they want to update the code to be fast, or they prefer it to be slow but work immediately. Does that sound reasonable?

Re migration from Solid: maybe that's something for a v1 release, or when we have a website, at the moment I don't really care about having more people using Voby/Oby, I don't have the bandwidth to write proper docs, a website, guides, and stuff like that. Also I can't write "migration from *" guides for every framework, and I basically consider Solid's stores unusable, there are so many issues with them that they are basically unusable really, imo.

Re performance: there's a WeakMap to cache proxies, we need something like that or the implementation wouldn't even be correct, besides being slow. I had benchmarked Voby's stores against Solid's stores at one point, and Voby's stores were faster. But the comparison is kinda pointless imo because Voby's stores to the best of my knowledge actually work, Solid's don't really, so the comparison doesn't make too much sense, like I can make anything as fast as you want if I can make it incorrect.

[fabiospampinato]

Regarding the 1 level unwrapping: depending on how you look at it it feels like a worst of both worlds, or a best of both worlds, compromise. From my point of view unproxying at 1 level is both potentially way slower than the constant time unproxying, and also doesn't solve the problem in general. From another point of view I guess it could make more code work out of the box with no user-level changes. I don't really like it as a solution.

[theSherwood]

Re something ought to be done

What you've outlined seems pretty reasonable. I think a big warning in the readme/docs would be very handy. This issue has cost me a lot of time because the nature of the bug is so weird. It's seemingly recursive but it never blows the stack. And I think there might be something in the oby code swallowing errors because when I was playing around with it locally, I threw errors from the oby code when the it went through too many iterations of get from the store and the recursion would stop but I'd never see the errors in the console. So that might also be something to hunt down now that there is a smallish repro. I may have had something else in the app catching those errors. I'm not sure.

Re migration from Solid: maybe that's something for a v1 release

I totally get that. Solid's stores have bitten me repeatedly. Especially as they've changed several times.

Re performance: there's a WeakMap to cache proxies

That makes sense.

Regarding the 1 level unwrapping

I was only thinking about this for when an array gets set on a store field. Any other unwrapping would be constant-time. But I agree that it is surprising and could lead users to think that the unwrapping is going to be deep when it isn't. At least with no unwrapping of array items (status quo), it's very clear that there's no unwrapping beyond what you see.

Thanks for all your time on this. Oby's batching is chef's kiss by the way. batch and tick are a great combo.

[fabiospampinato]

If I throw inside the effect I see the error in the console. Maybe you had an error boundary or something?

[theSherwood]

I was throwing from within oby library code. From within get proxy trap after it had been called 1000 times. I didn't see an error, but it's entirely possible I had an error boundary. I know the test framework I'm using swallows the errors in some circumstances.

[theSherwood]

An option that just occurred to me is to have a debug mode that catches this infinite loop. Not clear to me how much of a lift that would be, though.

[fabiospampinato]

I'm going to add a warning to the readme about this. I'm not adding the { deep: true } version of store.unwrap at this point in time mainly because that would make store.unwrap harder to use, because you wouldn't be able to do something like this anymore: [a, b, c].map ( store.unwrap ), and it would be a breaking change, and I don't know if I want to ship a store.deepUnwrap function instead, but I'm keeping it in mind for the future.

---

In the meantime if anybody needs a deepUnwrap, this should do the trick:

import {store} from 'oby';

const isProxiable = ( value: unknown ): boolean => {
  if ( value === null || typeof value !== 'object' ) return false;
  if ( isArray ( value ) ) return true;
  const prototype = Object.getPrototypeOf ( value );
  if ( prototype === null ) return true;
  return ( Object.getPrototypeOf ( prototype ) === null );
};

const deepUnwrap = <T> ( value: T ): T => {
  const unwrapped = store.unwrap ( value );
  if ( isProxiable ( value ) ) {
    if ( Array.isArray ( value ) ) {
      const clone = new Array ( value.length ) as T;
      for ( let i = 0, l = value.length; i < l; i++ ) {
        clone[i] = deepUnwrap ( value[i] );
      }
      return clone;
    } else {
      const clone = {} as T;
      for ( const key in value ) {
        clone[key] = deepUnwrap ( value[key] );
      }
      return clone;
    }
  } else {
    return unwrapped;
  }
};

[fabiospampinato]

@theSherwood are you still using Oby? Just curious

[theSherwood]

I am currently still using it. The batching/tick behavior is pretty essential to the current iteration of my app. Though I've recently had some insights that might let me take over the tick behavior that would let me use alternate renderers. But I'm not in a rush to switch.