omniscientjs / immstruct

Immutable data structures with history for top-to-bottom properties in component based libraries like React. Based on Immutable.js
374 stars 21 forks source link

Maximum call stack size exceeded #65

Closed hokkos closed 9 years ago

hokkos commented 9 years ago

Hi, here is a little sample that crash the call stack in the current 1.4.0 and the master one. I want to swap 2 element in an List and modify their content.

var structure = Immstruct('a', [ {a:1}, {b:2}]);
var cursor = structure.cursor();
var ca = cursor.get(0);
var res  = cursor.withMutations(function(c){
      c.set(0, cursor.get(1))
      .set(1, ca)});
structure.cursor().setIn([0, 'b'], 3)

This raise a stack overflow error. Any idea ? Thanks.

mikaelbr commented 9 years ago

There might be that there is a circular structure dependency happening here in immstructs update function (merging). I checked the equivalent with pure Immutable.js (without cursors), and it seemed to work. The same goes for "vanilla" cursors.

mikaelbr commented 9 years ago

As I suspected, there is a circular dependency between a and b caused by something in the immstruct code and perhaps persistant data structures.

Doing something like this would work:

var structure = immstruct([ {a:1}, {b:2}]);
var cursor = structure.cursor();
var ca = cursor.get(0).deref();

var res  = cursor.withMutations(function(c){
  c.set(0, cursor.get(1).deref())
   .set(1, ca)
});

structure.cursor().setIn([0, 'a'], 3);

Need to dive more into this to find where the actual stack overflow occurs.

hokkos commented 9 years ago

Yes, it works now, so did I misused the api and should have .deref() or is it a bug in the lib ? Thanks.

mikaelbr commented 9 years ago

It's a bug in the library that either refers to old structure and as the places has swapped it causes circular references, or there is something else that causes it.

It might actually be related to #45 if we're "lucky" (edit, probably not, misremembered the issue)

dashed commented 9 years ago

@hokkos cursor.get(0) and cursor.get(1) return Cursor instances; not Immutable.Map:

    /**
     * Returns the value at the `key` in the cursor, or `notSetValue` if it
     * does not exist.
     *
     * If the key would return a collection, a new Cursor is returned.
     */
    get(key: any, notSetValue?: any): any;
mikaelbr commented 9 years ago

Ah, of course, @Dashed. I didn't quite make sense of it while debugging right now. So yeah, by swapping the two data points with their cursors, you get two references which causes the circular dependency between them self and causing the infinite indirect recursion by Immutable.js cursors implementation.

@hokkos So, this is a usage error, not a bug. Didn't think about Cursor#get returning a Cursor. You want to update to insert the data structure not the actual cursors, so doing deref as I showed you would be correct.

hokkos commented 9 years ago

Thanks.