Flotype / now

NowJS makes it easy to build real-time web apps using JavaScript
http://www.nowjs.com
MIT License
1.91k stars 175 forks source link

Maximum call stack size exceeded #143

Closed MSNexploder closed 13 years ago

MSNexploder commented 13 years ago

The following code dies using node v0.4.12. I was able to track down the problem within the nowUtil.extend function, which gets recursively for deeply nested objects until stack size exceeds. More precisely it looks like it couldn't handle circular references properly.

But I couldn't come up with a not too hacky solution for this.

var io = require('socket.io');
var now = require('now');
var express = require('express');

var server = express.createServer();
var store = new io.RedisStore();

var everyone = now.initialize(server, {socketio: {store: store}});
// => RangeError: Maximum call stack size exceeded
ericz commented 13 years ago

Indeed the extend function doesn't currently handle circular references correctly causing the error. Not sure if we're going to fix this atm

steveWang commented 13 years ago

Eric -- as a quick fix (and the least hacky method -- decycle / retrocycle is not really a good option), would you consider changing it such that nowjs.initialize modifies the input object, as opposed to the default settings object (how I believe things are currently done)?

I haven't looked at the code in some time, but this might be as simple as reversing the order of the arguments to nowUtil.extend.

edit: it's slightly more complicated. I think the fix would be to change nowUtil.extend(this.options, options); to this.options = nowUtil.extend(options, this.options);.

ericz commented 13 years ago

Yeahhh you want the defaults to be overwritten so it has to extend into this.options or extra memory will be used

steveWang commented 13 years ago

Erm, all right, it's not as simple as this, but the main idea is there. You'd actually want to use a slight variant of extend that only adds fields if they didn't already exist in the target.

I think it might be something like the following:

function recurse(source, dest) {
  for (var i in source) {
    if (source[i] && typeof source[i] === 'object') {
      if (!dest[i] || typeof dest[i] !== 'object') {
        dest[i] = Array.isArray(source[i]) ? [] : {};
      }
      recurse(source[i], dest[i]);
    }
    else if (dest[i] === undefined) {
      dest[i] = source[i];
    }
  }
}

Also, sorry, I don't follow -- does it really use more memory if you extend into the other object, then overwrite this.options?

ericz commented 13 years ago

makes sense. i'll implement that

On Tue, Oct 4, 2011 at 8:33 AM, Steve Wang < reply@reply.github.com>wrote:

Erm, all right, it's not as simple as this, but the main idea is there. You'd actually want to use a slight variant of extend that only adds fields if they didn't already exist in the target.

Reply to this email directly or view it on GitHub: https://github.com/Flotype/now/issues/143#issuecomment-2287717

510-691-3951 EECS Student at UC Berkeley http://ericzhang.com

ericz commented 13 years ago

Fixed