RangeError: Maximum call stack size exceeded

[Macil]

This code fails with "RangeError: Maximum call stack size exceeded" after a short time.

var Bacon = require('baconjs');

function step() {
  return Bacon.later(1, 'beep');
}

function loop() {
  var s = step();
  return s.merge(s.delay(1).flatMap(loop));
}

loop().log();

Seems like I independently ran into this issue, but I don't think an issue was opened here for it.

[raimohanska]

True that. Infinitely recursive flatMap doesn't work. There's no easy "fix" for this. Pull Requests welcome!

[Macil]

I ended up working around it in my code like this:

var Bacon = require('baconjs');

function looper(fn, delay) {
  return Bacon.fromBinder(function(sink) {
    var unsub, timer;
    function sub() {
      unsub = fn().subscribe(function(event) {
        if (!event.isEnd()) {
          sink(event);
        } else {
          timer = setTimeout(sub, delay);
        }
      });
    }
    sub();
    return function() {
      unsub();
      clearTimeout(timer);
    };
  });
}

function step() {
  return Bacon.later(1, 'beep');
}

looper(step, 1000).log();

Changing the 1000 down to 1 or 0 quickly shows that it doesn't blow up. looper might be general enough to be included in Bacon I think. It also has the benefit of handling step streams that emit more than one event each.

EDIT: This function has flaws with being unsubscribed from. See my later version of it for fixes.

[phadej]

For now this works:

var Bacon = require('bacon.js');

function step() {
  return Bacon.later(1, 'beep');
}

function fix(f) {
  var bus = new Bacon.Bus();
  var result = f(bus);
  bus.plug(result);
  return result;
}

var loop = fix(function (r) {
  return step().merge(rec.delay(1));
});

loop.log();

Related to #507. Recursive .flatMap is hard, but fix could turn out to be easy.

[rpominov]

+1 for including looper() to the core. A possible use case:

var animationFrames = looper(function() {
  return Bacon.fromCallback(requestAnimationFrame);
});

Not sure about the delay argument though. I think it should be optional at least, so if it not provided there is no setTimeout used.

[rpominov]

Here how it might look without timeout:

function looper(fn) {
  return Bacon.fromBinder(function(sink) {
    var unsub;
    function sub() {
      unsub = fn().subscribe(function(event) {
        if (!event.isEnd()) {
          sink(event);
        } else {
          sub();
        }
      });
    }
    sub();
    return function() {
      unsub();
    };
  });
}

Edited: removed not needed manual if (!deactivated) check before calling sub()

[Macil]

The fix solution is neat, though it doesn't handle unsubscription (and currently fails on step streams that emit synchronously because of #520).

The looper without a timeout still overflows the stack if used with a step stream that emits synchronously (sync streams seem like the main reason to me you'd want to take out the setTimeout, so it's a bit self-defeating). Also, adding a timeout before the end of the stream in order to add in some time between steps seems a bit awkward to me, but maybe it's fine (though also fails on synchronously-emitting streams because of #520):

var Bacon = require('baconjs');
var looper = require('./looper-no-timeout');

function step() {
  return Bacon.later(1, 'beep');
}

looper(function() {
  var s = step();
  return s.merge(s.mapEnd(null).delay(1000).filter(false));
}).log();

[rpominov]

This version of your last example should work fine, but it still a bit awkward I agree.

function step() {
  return Bacon.later(1, 'beep');
}

looper(function() {
  return step().concat(Bacon.later(1000).filter(false));
}).log();

[Macil]

In my project, step is an AJAX request that takes a variable amount of time, and I want to wait a certain amount of time after one request ends before starting a new one. After more thought, I'm fine with the s.merge(s.delay(1000).filter(false)) part. It's mainly just the fact that the timeout-less looper fails to solve the problem it was made to solve (stack overflows) for the case of synchronously-emitting streams that makes me uncomfortable. I would prefer a timeout-less looper if it didn't fail there. (Not that I'm the biggest fan of synchronously-emitting streams, but I keep accidentally running into them and their edge cases, so I try to plan for that.)

[Macil]

Oh, I had missed your example about using looper with requestAnimationFrame. That's pretty convenient.

It sounds like from other bugs that synchronously-emitting streams have problems of their own (and are hopefully on the chopping block?), so worrying about them shouldn't block a timeout-less looper.

Though I just realized it's pretty trivial to check for synchronously-emitting streams (by checking if .subscribe has returned), and only use a timeout in that case.

var Bacon = require('baconjs');

function looper(fn) {
  return Bacon.fromBinder(function(sink) {
    var stopper = new Bacon.Bus();
    var stopperProp = stopper.toProperty();
    // force evaluation of stopperProp so it remembers its value
    stopperProp.onValue(function() {});
    function sub() {
      var subscribeHasReturned = false;
      fn().takeUntil(stopperProp).subscribe(function(event) {
        if (!event.isEnd()) {
          sink(event);
        } else {
          if (subscribeHasReturned) {
            sub();
          } else {
            // If the fn() stream emitted synchronously, we should loop
            // asynchronously to avoid blowing up the stack.
            Bacon.later(0).takeUntil(stopperProp).onValue(sub);
          }
        }
      });
      subscribeHasReturned = true;
    }
    sub();
    return function() {
      stopper.push(null);
    };
  });
}

// Pathological step function that needs this more defensive unsubscription logic
// used in looper in order to be unsubscribed from correctly.
var i = 0;
function step() {
  if (++i == 1) {
    return Bacon.later(1, 'later');
  } else {
    return Bacon.once(5).merge(Bacon.repeatedly(200, [6,7,8]));
  }
}

looper(step).take(2).log(1);

It took me quite a few edits to get the unsubscription logic fully down. (My original version had the issues too.) I tried keeping the return value of .subscribe in a variable and calling it in fromBinder's unsubscribe callback, but it wouldn't always be set with the correct value since the looper stream could be unsubscribed from before the latest call to .subscribe had returned. I tried keeping a flag variable around to remember the unsubscription and then act appropriately in the next time .subscribe's callback was called, but then that meant we'd have to wait on one more event from the step stream before we could tell it we were unsubscribing. A bus property ended up being the sanest and best-working approach.

[rpominov]

Yeah, in the requestAnimationFrame I was trying to show that looper is a nice abstraction, that Bacon currently lacks. I think it is general and usable enough to be in the core.

But there is obviously some implementation issues with synchronous observables. Not sure what is desirable behavior here should be, one of the options btw is to just throw an exception saying "you can't use looper with synchronous observables".

[Macil]

Throwing an error could be undesirable since the given step function might only emit synchronously occasionally at run-time depending on the application. (Might be a little odd of a case, but think of properties which may or may not have a current value.) I think the gracefully degrading behavior as implemented above where it falls back to timeouts for synchronously-ending observables is better and can be easily documented. (If you phrase it as that multiple step calls are never made in the same event loop turn, then it sounds pretty natural.)

[rpominov]

Here is another shot on looper function. The recursion replaced with iteration, and unsubscription done via Bacon.noMore.

The only problem with this implementation is: if user will do something like looper(function(){return Bacon.once(1)}).log() it will stuck in infinite loop. But it probably ok, since simple while operator has same feature and it doesn't considered as an issue.

function looper(step) {
  return Bacon.fromBinder(function(sink) {

    var hasSource, inLoop, noMore = false;

    function handleEvent(event) {
      if (!event.isEnd()) {
        if (Bacon.noMore === sink(event)) {
          noMore = true;
          return Bacon.noMore;
        }
      } else {
        if (inLoop) {
          hasSource = false;
        } else {
          iterateSubscribe();
        }
      }
    }

    function subscribe() {
      hasSource = true;
      step().subscribe(handleEvent);
    }

    function iterateSubscribe() {
      hasSource = false;
      inLoop = true;
      while (!hasSource && !noMore) {
        subscribe();
      }
      inLoop = false;
    }

    iterateSubscribe();

    return function() {};

  });
}

[Macil]

That looper function doesn't handle unsubscription here:

var i = 0;
function step() {
  if (++i == 1) {
    return Bacon.later(1, 'later');
  } else {
    return Bacon.once(5).merge(Bacon.repeatedly(200, [6,7,8]));
  }
}

looper(step).take(2).log(1);

The node process doesn't exit because the step stream isn't unsubscribed from.

[rpominov]

It does after #523 fixed in v0.7.42 ;)

[Macil]

Oh good, I think I tried something like that before but I gave up trying to figure out if there was a bug there or not.

[raimohanska]

Ok guys, there's a new implementation of Bacon.retry in master, that does not do stack overflow. It's implemented on top of the new method Bacon.fromStreamGenerator that is a slightly modified version of @pozadi's looper. The main difference is that the "step" function can return undefined to tell that no more iterations are to follow.

Maybe we should replace undefined with Bacon.noMore. So "step" could return either a stream or noMore... Dunna. What do you think? In any case, I think the new retry is better than the old one :)

[raimohanska]

So the new method Bacon.fromStreamGenerator is in master and has tests and all, but is undocumented. Before documentation I hope we could agree on its eventual interface. The impl is pretty simple: https://github.com/baconjs/bacon.js/blob/master/src/fromstreamgenerator.coffee

And the tests hopefully demonstrate how it's used: https://github.com/baconjs/bacon.js/blob/master/spec/specs/fromstreamgenerator.coffee

[rpominov]

I was thinking about this lately too, and it's funny that I came up with same ideas, both that .looper is pretty similar to .retry, and that it would be good to be able to return false or something from generator function to end the loop.

Also maybe it would be useful to pass iteration number to the generator — would be easier to implement .retry-like logic inside of .fromStreamGenerator. But not sure about this, since there is .retry already in Bacon.

I didn't understand completely how .fromStreamGenerator works yet. Implementation looks very simple, but I noticed there is no iteration in it, so there have to be recursion. Is that intentional? Don't you want to remove recursion from it, so it can be used without possibility of getting "Maximum call stack size exceeded"? Or perhaps I'm missing something here.

[raimohanska]

@pozadi you're right that it'll go recursive with syncronous streams returned from the stepper, and can thus lead to stack overflow. Your implemenation above seems to cleverly solve this issue with the flag variables. It would certainly make sense to adopt that technique :)

Passing iteration number might be a good idea and simplify the implementation of some stepper functions. Also, I wish there was a way to get rid of the variables "error" and "finished" in the retry implementation. Iteration number is not quite enough for that.

And then there's the issue of naming this thing: fromStreamGenerator is a bit long and looper might be quite catchy.

[raimohanska]

Oh btw it seems that fromStreamGenerator passes @agentme's test above. Below an adapted version.

Bacon=require("../dist/Bacon")

var i = 0;
function step() {
  if (++i == 1) {
    return Bacon.later(1, 'later');
  } else {
    return Bacon.once(5).merge(Bacon.repeatedly(200, [6,7,8]));
  }
}

Bacon.fromStreamGenerator(step).take(2).log();

Outputs

later
5
<end>

[rpominov]

How about loop, repeat, of respawn for method name?

BTW, .repeatedly can be built on top of .sequentially and this method:

Bacon.repeat(function() {
  return Bacon.sequentially(1000, [1, 2, 3]);
});

[raimohanska]

@pozadi, I made do with a single boolean flag :) fdb9bdd

[rpominov]

Power of CoffeeScript, I guess ;)

[rpominov]

Noticed another problem with all implementations here. Suppose we created a steam using looper, then we subscribed to it, got some events, unsubscribed, and later subscribed again. On second subscription the generator function will be called again although the stream it returned first time maybe still alive.

I think the correct behavior is that we should subscribe again to previous generated observable, if it didn't end yet instead of generating new one.

[Macil]

Shouldn't unsubscribing from the looper cause the stream it returned to be
unsubscribed too? That's what most of my test cases in the thread have been
testing.
On Feb 14, 2015 12:41 PM, "Roman Pominov" notifications@github.com wrote:

Noticed another problem with all implementations here. Suppose we created
a steam using looper, then we subscribed to it, got some events,
unsubscribed, and later subscribed again. On second subscription the
generator function will be called again although the stream it returned
first time maybe still alive.

—
Reply to this email directly or view it on GitHub
#521 (comment).

[rpominov]

Yep, the problem is, we won't subscribe to it back, when the looper get a subscriber again. Instead we'll generate new one via generator function.

[Macil]

Oh, I never imagined it working that way. It doesn't impact my use case at
least. (My original use case was using looper with a function that returned
a stream that started an Ajax connection, emitted the results, and waited
30 seconds after getting the response before ending. Reusing any of those
generated streams wouldn't do anything special for me.)
On Feb 14, 2015 12:58 PM, "Roman Pominov" notifications@github.com wrote:

Yep, the problem is, we won't subscribe to it back, when the looper get a
subscriber again. Instead we'll generate new one via generator function.

—
Reply to this email directly or view it on GitHub
#521 (comment).

[raimohanska]

End result: Bacon.repeat and Kefir.repeat added!