Infinite loop when subscription immediately dispatches

[mrozbarry]

I have a Countdown subscription, which for simplicity, looks like this:

const CountdownFx = (dispatch, { id, seconds, progress, complete }) => {
  let remaining = seconds + 1; // this is contrived to make this example smaller
  let handle = null;
  const tick = () => {
    remaining -= 1; // again, contrived
    if (remaining > 0) {
      dispatch(progress, { id, remaining });
      handle = setTimeout(tick, 1000);
    } else {
      dispatch(complete, { id });
    }
  }

  tick();

  return () => clearTimeout(handle);
}

export default props => [CountdownFx, props]

When I enable this countdown, it triggers setState immediately, which runs subscriptions immediately, and does an infinite loop.

If I make a timeout around the initial tick, everything works fine.

My real example makes more sense that tick happens immediately, but this example also triggers the problem.

---

The problem is here.

A solution (which is maybe incorrect) is to mimick the render deferring for subscriptions, where running the next subscription patch is deferred/locked.

[jorgebucaran]

We deferred subscriptions before, but it created other problems.

/cc @zaceno @sergey-shpak

See also:

• https://github.com/jorgebucaran/hyperapp/issues/844
• https://github.com/jorgebucaran/hyperapp/issues/844

[zaceno]

A solution (which is maybe incorrect) is to mimick the render deferring for subscriptions, where running the next subscription patch is deferred/locked.

A lock mechanism on patchSubs which prevents infinte looping, but without deferring to next tick, should be possible I think, and would solve the problem for @mrozbarry as well.

By lock mechanism I mean a recursion guard. basically:

if (already entered this function but didn't leave it yet) return

[jorgebucaran]

By lock mechanism I mean a recursion guard.

I'd love to see that in practice in a PR.

[mrozbarry]

We deferred subscriptions before, but it created other problems.

Just for context, what problems did it create?

[mrozbarry]

@zaceno not sure that guard will work if there is a legitiment state update that should either (re)start or cancel another subscription.

[zaceno]

@mrozbarry Hmm, yeah you're right. But deferring won't help in that case either. It will just be a slightly slower-spinning infinite loop, won't it?

So if a subscription can dispatch immediately, it needs to be up to the subscription itself to prevent infinite looping, I guess. Regardless of deferral.

EDIT: Oh wait you're right. Deferring would mean we actually check the new subscriptions for equality, before calling the subscriptions again. So that would fix it.

The reason I wanted patchSubs not deferred was because I wanted the first run to be in the same tick as init. One example of why this would be good, would be if you have a route-subscription. Right when you start the subscription (probably from the start of the app), you could have an immediate dispatch of the current route. So right from the first render, you have the current route in the state.

[mrozbarry]

@zaceno even that is a problem. If you have a routing subscription that immediately dispatches a new action/state, the patchSub goes into an infinite loop.

Looks like my current solution in a local branch (I'll PR momentarily) is to pass the sub a dispatch that is deferred by 0ms. It's not the greatest, but it doesn't drop updates, and it doesn't lock the app up.

I think either way, something has to be deferred. We either have to very clearly document that the first dispatch has to be deferred, or defer the sub until after patchSub has ran.

I looked into deferring the sub execution, but I think that requires a lot of new code that will bloat Hyperapp (ie something that can inject cancel functions for subs later).

[sergey-shpak]

@mrozbarry some history context: as @jorgebucaran already mentioned I had an issue with deferred subscriptions (https://github.com/jorgebucaran/hyperapp/issues/844) (in my environment deferred based on 'requestAnimationFrame' is never called, as result deferred render and subscriptions are never initialized). To solve the issue we decided to move subscriptions outside from deferred render function (it makes a lot sense, because subscriptions should not always depend on render)

Since dispatch and subscriptions are sync now, immediate dispatching from any subscription will create infinite loop - it is by design. My point is hyperapp#2 should not fix such kind of issues, this has to be done by function itself (which creates the loop). Comparable example is simple recursive increment function:

// infinite loop created and stopped by 'inc' function
function inc(val){return val < 10 ? inc(val++) : val}

This means that stopping infinite loop has to be done on the subscription side,

let initialized = false // or any state property
const sub = (dispatch, { action }) => {
  if(!initialized) {
    initialized = true
    dispatch(action)
  }
}

[zaceno]

@sergey-shpak The inc recursion example you gave is not quite the same as what's happening here.

It is true, that if I have a subscription which immediately dispatches an action, which changes the state in such a way that the subscription props change, then an infinte loop is the obvious behavior.

The problem discussed here is this:

The way subscriptions are meant to work is that when the subscriptions: function returns an identical array as last time, no subscriptions should be started or stopped. That is how we expect subscriptions to work. But for some reason, when your subscriptions-function dispatches immediately, the subscription function is called again (dispatching immediately again ... and so on). Despite the fact that the subscription-array is not changed.

EDIT: As an example:

I expect this to cause an infinite loop, for obvious reasons:

const mySubFn = (disp, state) => {
  disp(myAction, state + 1)
  return () => {}
}

const myAction = (state, news) => news

app({
  init: 0,
  subscriptions: state => [[mySubFn, state]],
  ...
})

I don't expect this to cause an infinite loop, but it does:

const mySubFn = (disp) => {
  disp(myAction)
  return () => {}
}

const myAction = state => state + 1

app({
  init: 0,
  subscriptions: state => [[mySubFn, 'foo']],
  ...
})

It should not cause an infinite loop, because the subscriptions don't change for each state update, so the mySubFn should not be called again. However we solve this, wether we defer or not, this behavior is clearly a bug imo.

[sergey-shpak]

@zaceno got it,

// 'sub' is some subscription

// this won't reinitialized because subs are exactly the same array each time
const subs = [ [sub] ]
app({ subscriptions: subs })

// this is reinitialized each time, because you're creating each time new array
app({ subscriptions: [[ sub ]] })

this sounds like some kind of subissue and is more related to how subs are patched than to infinite loop issue (I've already discussed this issue with Jorge at slack channel)

[zaceno]

@sergey-shpak

even using a constant const subs = [[sub]] and then subscriptions: state => sub does not work the way it should:

https://codepen.io/zaceno/pen/RzYGPW

... still produces a Maximum call-stack exceeded

[jorgebucaran]

@zaceno Thanks, this creates an infinite loop because subscriptions are currently sync. They used to be async, but we switched over to sync. Would async be preferred, then?

[zaceno]

@jorgebucaran

this creates an infinite loop because subscriptions are currently sync

Ok but that's not really an explanation you could use in docs.

Based on my understanding of how subscriptions are supposed to work, mySubFn in the example below should be called exactly once.

const mySubFn = (disp) => {
  disp(myAction)
  return () => {}
}

const myAction = state => state + 1

const staticSubs = [[mySubFn, 'foo']]

app({
  init: 0,
  subscriptions: state => staticSubs,
  ...
})

...regardless of wether subscriptions are sync or async.

It's the core of how subscriptions are supposed to work: if the subscriptions don't change, we don't call anything to start/stop them.

What I'm saying is: there's no way this can be considered the expected and intended behavior. This must be a bug.

Would async be preferred, then?

First, I would prioritize fixing this bug whichever way. And perhaps adding a test to prevent future regressions.

If the only way to fix this bug is to go back to async, then do that. But I would prefer if we can fix this bug while keeping subs sync. I believe it is possible but I haven't had time to look into it or PR anything.

[jorgebucaran]

@zaceno I don't know what's the best way to behave here. For example, in Elm subscriptions is called immediately after update; they're not handled async as far as I know. I also haven't had problems implementing any of the official subscriptions at this point, but that could change.

@mrozbarry's use case could be considered a misused of subscriptions (not saying that yet, though) and is easy to resolve by dispatching actions in the next tick.

[zaceno]

@jorgebucaran just forget for a moment about sync/async, and look at that example. Based on how you intended the subscriptions property to work, when would you expect mySubFn to be called?

[jorgebucaran]

Hmm, right now, I'm going to go with yes, for the same reason I'd expect Hyperapp to go into an infinite loop if I could dispatch an action with the view like:

app({
  view: state => {
    dispatch(Foo) // Imaginary example!
    return h("h1", {}, "hello")
  },
  // ...
})

What do you think?

[zaceno]

Well that comparison isn’t valid, because subscriptions aren’t meant to be started every state update.

Surely you’re not saying that the fact that my subscriptions are constant doesn’t matter?

[zaceno]

Actually I thought some more about it, and I'm no longer sure this bug can be fixed and have not-deferred patchSubs. The reason the recursion occurs is because we run through patchSubs again and again comparing the new subscriptions array with the original (empty) subscriptions array. So of course we'll keep calling mySubFn. over and over. We never get to the point where we know it was already started.

The only way around that is to defer the next patchSubs until we know all the subscriptions have been started.

So rather than belabor the point that this is a bug and not expected beahvior, I'll just say: let's go back to async.

[jorgebucaran]

@zaceno Not 100% sure, yet. It's not a problem to go back to async, but I'm going to look at @mrozbarry's example more closely. For starters, it's the only example we have here with some meat. dispatch(myAction) is not a real subscription use case.

[zaceno]

@jorgebucaran

You're right -- better to look at real use cases. In case you're interested, here's my example I would like to work (as it is written, without needing to change any of my code)

https://codepen.io/zaceno/pen/qzKWPY?editors=0010

Another way of phrasing the point I'm trying to make is: my subscription implementation shouldn't need to keep track of wether it has been started or not, because supposedly Hyperapp is managing that for me.

---

UPDATE:

Since this thread concluded (at the time of writing) that immediate dispatching should never be done in subscriptions, and instead effects should be used, I made a version of the basic routing codepen above using that approach instead:

https://codepen.io/zaceno/pen/YoOmvw

[jorgebucaran]

@zaceno Looking at it. Off-topic, but could you use this to declare your subscriptions?

const fx = a => b => [a, b]

const MySub = fx((dispatch, props) => {
  ...
  return () => { ... }
})

[zaceno]

@jorgebucaran Sure thing. I avoided it at first to keep it "unfancy" and distraction free. Updated now :)

[sergey-shpak]

just to clarify,

• first issue, with subscriptions which produces infinite loop when action dispatched immediately (caused by synced subscriptions)
• second issue, passing each time new array (with the same values), which creates subscription reinitialization on each setState (caused by patching subscriptions)

which issue we are discussing?

[zaceno]

@sergey-shpak I was discussing the first issue. The second I have not run up against.

[sergey-shpak]

ok clear, so to solve the first issue we have several options:

1. control immediate dispatching by flag or use deferred dispatch inside subscription (setTimeout)
2. return to async subscriptions
3. use init action to compose immediate actions

I would go with 3, your example:

const SetRoute = (state, route) => ({...state, route})
const OnRoute = fx((dispatch, action) => {
    const handler = () => dispatch(action, window.location.hash || '#')
    window.addEventListener('hashchange', handler)
    return () => window.removeEventListener('hashchange', handler)
})
//...
app({
    node: document.body,
    init: [setRoute, window.location.hash],
    subscriptions: state => [OnRoute(SetRoute)],
    view: state => h('body', {}, ROUTES[state.route].view(state)),
})

[jorgebucaran]

@sergey-shpak This issue is about sync/async subscriptions.

@zaceno Rather than dispatching the hashchange action by force, your app should be initialized with the hash state from the start: init: { route: window.location.hash || "#" }.

I believe that is the right approach, the problem is how to reduce boilerplate. One way is with an HOA: https://codepen.io/anon/pen/agayzP.

EDIT: @sergey-shpak beat me to it. 🙌

[zaceno]

@sergey-shpak, @jorgebucaran

Of Sergey's options I prefer nr 2 and nr 3 together. In other words, I think we should go back to async, so that we don't cause infinite loops in for subscriptions that dispatch immediately.

Doing that means I will need to use init (option number 3), instead of immediate dispatch to set the route right before my codepen renders. Thats... acceptable. It's not what I was hoping for but it's looking like the only realistic option.

So why do I care about number 2 (async subs), if I'm not going to use immediate dispatch anyway?

• It still seems fragile that we can throw hyperapp into an infinite loop so easily. It's surprising. At the very least we should write in the docs: "whatever you do, don't dispatch in the same tick as the subscription is called, because that will throw apps into an infinite loop". Unless we document such a warning, the current behavior is a bug in my opinion.

So why go back to async, rather than just document the warning and move on?

• Because if we go back to async we won't have to document any special cases/caveats. And that is a plus.

But wasn't it I who asked for sync subscriptions in the first place?

• Yes, I was at least one of those who suggested it. The reason was that I was hoping for something that didn't work out. I still have some faint hope it could be possible, but my hope is waning. The hope was that I would be able to get data into the state before the first render, using only a subscription rather than a combination of init + subscription.

[frenzzy]

What if provide to subscriptions not the original dispatch function but a function which will remember with which arguments it was called and call dispatch after all subscriptions are executed synchronously?

[zaceno]

@frenzzy Would that achieve what I was hoping originially: "be able to get data into the state before the first render, using only a subscription rather than a combination of init + subscription." ?

[frenzzy]

Yes, I think so. But I am not sure a subscription is the right way for it.

[sergey-shpak]

@zaceno imo subscriptions are designed for external async changes, dispatching immediate sync actions with subscriptions looks like not appropriate subs api usage (I believe init action is designed for this case). But I'm ok with both sync or async subscriptions (if it brings any real benefit to hyperapp#2 and doesn't raise once more https://github.com/jorgebucaran/hyperapp/issues/844 upd: anyway docs have to be updated with proper warning/description

[zaceno]

@sergey-shpak I can imagine a few types of subscriptions one might want, where you don't just want updates, but also the "current state" of whatever you're subscribing to. And that may not necessarily be at the point in time when your app inits. (Maybe you don't care about the sub right from the start).

Let's say, for example you want to keep track of the current window size. Just straight of the top of my head (not tested), I would try something like this first:

const fx = a => b => [a, b]
const WindowSize = fx((dispatch, action) => {
  const handler = () => dispatch(action, {
      width: window.innerWidth,
      height: window.innerHeight,
  })
  window.addEventListener('resize', handler)
  handler()
  return () => { window.removeEventListener('resize', handler) }
})
...
const SetSize = (state, winsize) => ({...state, winsize })
...
app({
  init: {tracksize: false, winsize: null},
  subscriptions: state => [
    state.tracksize && WindowSize(SetSize)
  ],
  ...
})

That won't work of course but it can be fixed:

 ...
  window.addEventListener('resize', handler)
  requestAnimationFrame(handler)
  return () => { window.removeEventListener('resize', handler) }
  ...

(EDIT: felt the need to verify my code: https://codepen.io/zaceno/pen/vqzpox?editors=0010)

Since I believe this type of use case of immediate dispatches – not only at init time! – is relevant, and since I feel like the first try would give a better DX, I think we should go back to async subs.

I still won't get what I originally hoped for: ability to prefill the state before first render using only a subscription. But we can't always get what we want anyway 😅

[jorgebucaran]

@zaceno There's no mystery. Also, let's not forget the old adage: you shouldn't be building subs anyway.

You need to defer your action with rAF (or setTimeout) in your sub for the same reason your original navigation subscription was going into an infinite loop. The solution is to use init to initialize the app with the data you need. In this case, with the window size and avoid a pointless re-render at the same time. Like @sergey-shpak said, subscriptions are designed for external async changes.

Here's another example. Let's say you're building a clock app. You're going to use @hyperapp/time to update your state every second. But how do you handle the first tick? By initializing the app with the current time.

Going back to async subs would let us have it both ways, but rather than using a shortcut, I'd prefer we use the approach that fits right in with the rest of the architecture.

[jorgebucaran]

From a convo with @zaceno in Slack. The following dialog illustrates why immediately dispatching actions from a sub is the wrong approach (not to mention you may create an infinite loop).

Jorge: It's as if you wanted: addEventListener("size", fn) to call fn once with the current window size and every time after the window size changes.

Zac: I just think it would be nicer if subs can give one callback right when they start. But I do see what you're saying. Subs mirror addEventListener and they should only call back when the actual event happens.

[zaceno]

So in conclusion:

• Things which require immediate dispatch should be implemented with effects.
• Therefore subscriptions should never need to do immediate dispatch
• Therefore it doesn’t matter if using immediate dispatch in subscriptions breaks or not
• Therefore this issue has no bearing on wether we make subs async or sync.
• The only people writing subscriptions should know what they’re doing.
• Hence it doesn’t matter if we document the problem with immediate dispatch in subs or not.

Is this correct @jorgebucaran ?

(I may not 100% buy into the assumptions, but I can follow the logic at least.)

[jorgebucaran]

@zaceno Here are some answers:

Things which require immediate dispatch should be implemented with effects.

Yes, or without them. To avoid reading from the global scope, e.g., window, inside your init function, I suggest creating an HOA as I described in https://github.com/jorgebucaran/hyperapp/issues/857#issuecomment-508838282. What's nicer, an HOA to init the app with default parameters.

export const withDefaults = params => app => props =>
  app({
    ...props,
    ...{ init: () => props.init(params) }
  })

import { withDefaults } from ".."

const initialState = {
  /*...*/
}

app
  |> withDefaults({ hash: window.location.hash })
  |> {
    init: ({ hash }) => ({ hash, ...initialState }),
    view,
    subscriptions,
    node: document.getElementById("app")
  }

The only people writing subscriptions should know what they're doing.

People should know what they're doing whether they're writing subscriptions or not! :)

Hence it doesn’t matter if we document the problem with immediate dispatch in subs or not.

A note wouldn't hurt, but the new documentation I'm working on will be for typical users and getting things done quickly. Implementing your own effects/subscriptions will only make it to the API reference.

[okwolf]

@jorgebucaran the new documentation I'm working on will be for typical users and getting things done quickly. Implementing your own effects/subscriptions will only make it to the API reference.

I fully support this. I think separating out the implementation details of FX/subs will make the remaining docs more approachable.