Saga can't take action from one of its forks if the child dispatches synchronously inside the fork call

[MichalBures]

In my Electron library I'm using the store to communicate between sagas. Everything worked fine in 0.9.5, but after updating to 0.10.0 it broke. After some time trying to find out the problem it looks to me that takeEvery does not always work.

I haven't been able to replicate the problem in a simple example so far, though I was able to find out a specific moment in the library when it brakes. I put an action to a store (which works and can be seen in devtools), but the other saga that listens for that action with takeEvery isn't triggered. Keep in mind that the put is in a nested forked saga function.

I've tried to change all the forks to spawns since there were changes to the fork model, but that didn't help. What did help and fixed the problem was to put yield delay(0) before every put.

[yelouafi]

the noticeable change between 0.9.5 and 0.10.0 is that put effects are no longer scheduled in a Promise.then but are executed right away. Since it works with a delayit could be caused by the order takeEvery and put are issued.

You may try to put some console.log inside the sagas (the one that put and the other that takes) to check whether the take is issued before or after the put

[MichalBures]

Okay, I've found the situation that causes this problem.

export default function* RootSaga()
{
    yield* takeEvery('TEST', takeTest);
}


let wasTest = false;

function* takeTest(action) {
    if (wasTest) {
        console.log('Received TEST for second time')
    } else {
        console.log('Received TEST for first time')
        wasTest = true;

//      yield delay(0)
        yield put({type: 'TEST'})
//      yield delay(5000)
    }
}

The put in takeTest function isn't caught by the takeEvery without the delay(0). It looks like the takeEvery ignores the action dispatched from its forked saga.

Although If you uncomment the second delay and dispatch TEST from somewhere else during those 5 seconds it works correctly.

[yelouafi]

That's a curious case: takeEvery is forking a task, and the task is dispatching an action back to its forker. I'll be curious to know what's the use case here.

As for the issue:

• takeEvery internally calls fork(takeTest) and wait for the fork to return
• Since takeTest contains only non blocking calls it'll run fully synchronously inside the stack frame of fork(takeTest) including the put.
• It means the put is executed while takeEvery is still waiting for fork(takeTest) to return. takeEvery hasn't yet resumed to execute its take.

A possible solution is to enqueue the execution of forks like in #235 but i'll have to make sure this doesn't break the other tests. Until that you can get the same behavior of 0.9.5 by delaying the put, but instead of delay(0) I'd recommend a simple Promise.resolve() which is more performent.

[MichalBures]

While I was writing down the explanation for my use case I realized I've simplified the example too much and that's why it doesn't make much sense.

This is closer to what is actually happening:

export default function* RootSaga()
{
    yield [fork(takeEvery1), fork(takeEvery2)]
}

function* takeEvery1()
{
    yield* takeEvery('TEST', takeTest1);
}

function* takeEvery2()
{
    yield* takeEvery('TEST2', takeTest2);
}

let wasTest = false;

function* takeTest1(action) {
    if (wasTest) {
        console.log('Received TEST for second time')
    } else {
        console.log('Received TEST for first time')
        wasTest = true;

//      yield Promise.resolve()
        yield put({type: 'TEST2'})
    }
}

function* takeTest2(action)
{
//  yield Promise.resolve()
    yield put({type: 'TEST'})
}

In this example uncommenting either of those Promise.resolve() will make it work.

What I have in my library is a Global Action that carries another action as its payload and it's used to send actions between Electron windows. All you need to do is to create a Global Action, give it a target (name of the target window) and dispatch it to the store. The Global Action Handler saga then simply based on the action's target dispatches the carried action either locally with put or sends it to another window via ipc.

What happens is that sometimes an action received from another window will get an immediate response and another Global Action is dispatched back to that window. But the Global Action Handler is as you explained already busy waiting for that put to end and thus ignores that Global Action.

Simple schema of the chain:

IPC -> STORE -> GLOBAL ACTION HANDLER -> PUT -> ANOTHER SAGA -> PUT -> GLOBAL ACTION HANDLER -> IPC

PS: Thanks for that Promise.resolve() tip!

[joonhyublee]

@MichalBures I'm surprised your second example didn't work, because it is such a basic use case. Did 0.10.1 release fix it?

[MichalBures]

@joonhyublee Yes, 0.10.1 did fix this issue.

Although 0.10.1 did help with the previous example it's still not the same behavior as in 0.9. I've managed to isolate another one that doesn't work.

export default function* RootSaga()
{
    yield [fork(takeEvery1), fork(takeEvery2)]
}

function* takeEvery1()
{
    yield* takeEvery('TEST', takeTest1);
}

function* takeEvery2()
{
    yield* takeEvery('TEST2', takeTest2);
}

let testCounter = 0;

function* takeTest1(action) {
    if (testCounter === 0){
        console.log('Received TEST for 1. time')
        testCounter++;

        yield put({type: 'TEST2'})
    } else {
        console.log('Received TEST for ' + ++testCounter + '. time')
    }
}

function* takeTest2(action)
{
//  yield Promise.resolve()
    yield [fork(forkedPut1), fork(forkedPut2)]
}


function* forkedPut1()
{
//  yield Promise.resolve()
    yield put({type: 'TEST'})
}

function* forkedPut2()
{
//  yield Promise.resolve()
    yield put({type: 'TEST'})
}

This should receive TEST three times, but without either of those resolves it will receive it only two times.

I'm guessing it's related to the #291 that the two puts fire too fast and the takeEvery doesn't catch them.

[joonhyublee]

@MichalBures Thanks for sharing!

[joonhyublee]

@MichalBures @yelouafi So it seems, for now, that the only way to guarantee that all actions are caught is through using actionChannel (and I suggest you use it to see if you can get your test suite to work without hacky Promise.resolve()'s), as was in my case. While I have no problem making my app work with that, I don't think that is what users expect when they use an API named takeEvery..? ;) I'll have a more thorough read of #235 and source code, and see if I can come up with a suggestion for this.

[MichalBures]

@joonhyublee @yelouafi I can confirm that using actionChannel solves all the problems I had in both 0.10.0 and 0.10.1.

I would suggest adding support for channels in takeEvery so that when you have a complex use case that basic takeEvery can't handle you can simply add an actionChannel to it.

[yelouafi]

@MichalBures Thanks for sharing this. I think there is an error on the Fix of 0.10.1. I'll check

@joonhyublee
I dont think what we've here is a 'basic case'. Let me explain.

After switching redux-saga to a promise-less version, execution of non-blocking effects (Effects which execute and resume the Saga synchronously in the same stack frame just like a synchronous function call) no longer caused the Saga to sleep. Instead Saga resume asap as the function call returns and thus cause it not to miss events dispatched sync. For example if you do

dispatch(action) // Saga take this and execute it, then returns
dispatch(action) // after return 2nd dispatch executed then Saga take it again

As you see, a Saga doing only non-blocking calls will execute its effect fully inside the stack frame of the 1st dispatch, when the 1st dispatch returns, the Saga is already ready to take the 2nd. So as long as your flow of actions is unidirectional Saga can't miss an event (not talking of 0.10.1 b/c as I said there is an issue with the fix to fork)

There is one 'edge case', Suppose you have this fork/call chain

parent take(action) -> parent fork(child) -> ... -> child(n) put(action)

So here we have a cycle on the action flow. Even with that the parent Saga should have no problem with taking the action from one of its child. Except on an 'edge case' (of the former 'edge case'):

If all the call/fork chain is executing only non-bloking Effects, then the whole fork/call chain will execute as a single function call in the same stack frame. We'll have something like this

parent............................child.............

take(action)............................................
.............................................................. action is dispatched from outside
fork(child).............................................. fork Effect executed: begin of fn call
..............................put(action)............... child dispatch: fork han't yet returned
.............................................................. put returns
.............................................................. fork returns
take(action)........................................... Saga resumes

So the parent execute a normal function call and waiting for it to return. Except in the case of bounded recursion, I dont think (sync) cycles are basic cases on any kind of context I'm aware of.

The only way to fix the above is by making the nested put async. In other words by scheduling the nested put for execution later. And by later I mean until the function call to fork(child) terminate and the parent executes its next take effect (assuming the parent doing only non-blocking) (And from this POV, I don't consider Promise.resolve solution a hacky solution)

Prior to 0.10.0. the scheduling was done using Promise.then. which delayed all the puts to the next microtasks. But there was an issue with this solution: Promise.then was delaying the execution 'too much' (to the next microtask). It means the Saga doing put (here child) can miss any consecutive action dispatched on the current task.

This is why I implemented the solution in #235. My impl. delayed the nested puts only the time other Sagas are ready to take it. But the impl. of 0.10.0 handled only the cases of puts within puts not puts within forks. 0.10.1 attempts to fix this (but as I said the impl. of fix of 0.10.1 is incorrect, I'll have 'to fix the fix')

Fundamentally the re-entrant lock solution in #235 is somewhat similar to the actionChannel. But instead of queuing the actions we're queuing the functions dispatching those actions.

[yelouafi]

@MichalBures The new release 0.10.2 'fixes the fix' of 0.10.1. I added your example to the test suite (see https://github.com/yelouafi/redux-saga/blob/master/test/proc/takeSync.js#L470-527).

Could you try with the new release and see if it works?

[joonhyublee]

@yelouafi You are super awesome! Thank you for your clarification, it really helped me understand why it isn't really a 'basic use case', and I also realize saga scheduling is a difficult problem and doesn't 'just work'.

[MichalBures]

@yelouafi Yay, it works! Good job, takeEvery in 0.10.2 handles all my use cases again.

[yelouafi]

cool!

[axelson]

It looks to me like the current behavior of takeEvery in 0.10.5 still doesn't handle the example laid out in MichalBures comment on April 25th: #277 (comment)

I think this should be added as a note to the API documentation of takeEvery or change the implementation of takeEvery to add an automatic delay in the saga being executed. For now I am working around it by calling a fixedTakeEvery.

function * fixedTakeEvery (pattern, saga) {
  const worker = function * (...args) {
    yield Promise.resolve()
    yield fork(saga, ...args)
  }
  yield * takeEvery(pattern, worker)
}

The bottom line is that I don't expect takeEvery to miss any actions, even if those actions are created by the saga that is run by takeEvery. Also let me know if I should open this as a new ticket.

[yelouafi]

can you post a test case for this?

FYI heres is the test case for the previous fix:

https://github.com/yelouafi/redux-saga/blob/master/test/proc/takeSync.js#L470-L527

[axelson]

Yes here you go:

export default function * rootSaga () {
  yield fork(watchPing)
  yield put({type: 'PING', val: 0})
}

function * watchPing () {
  yield * takeEvery('PING', ackWorker)
  // yield * fixedTakeEvery('PING', ackWorker)
}

let first = true

function * ackWorker (action) {
  // yield Promise.resolve()
  if (first) {
    first = false
    yield put({type: 'PING', val: action.val + 1})
    yield take(`ACK-${action.val + 1}`)
  }
  yield put({type: `ACK-${action.val}`})
}

export function * fixedTakeEvery (actionType, saga) {
  const worker = function * (...args) {
    yield Promise.resolve()
    yield fork(saga, ...args)
  }
  yield * takeEvery(actionType, worker)
}

The above code results in PING, PING, ACK-1. But if you uncomment the yield Promise.resolve() or switch from takeEvery() to fixedTakeEvery() you will get the full sequence of PING, PING, ACK-1, ACK-0.

I tried to put this into an actual test case, but I'm getting different behavior (although the test is still failing).

test('inter-saga fork/take back from forked child 3', assert => {
  assert.plan(1);

  const actual = []
  const chan = channel()

  const middleware = sagaMiddleware()
  const store = createStore(() => {}, applyMiddleware(middleware))

  let first = true

  function* root() {
    yield [fork(watchPing)]
  }

  function* watchPing () {
    yield * takeEvery('PING', ackWorker)
    // yield * fixedTakeEvery('PING', ackWorker)
  }

  function* ackWorker (action) {
    // yield Promise.resolve()
    if (first) {
      first = false
      yield put({type: 'PING', val: action.val + 1})
      yield take(`ACK-${action.val + 1}`)
    }
    yield put({type: `ACK-${action.val}`})
    actual.push(1)
  }

  function* fixedTakeEvery (actionType, saga) {
    const worker = function * (...args) {
      yield Promise.resolve()
      yield fork(saga, ...args)
    }
    yield * takeEvery(actionType, worker)
  }


  middleware.run(root).done.then(() => {
    assert.deepEqual(actual, [1,1],
      "Sagas must take actions from each forked childs doing Sync puts"
    );
    assert.end();
  })

  store.dispatch({type: 'PING', val: 0})
  store.dispatch(END)
});

In the test case actual ends up with just [1] which is expected. But I would expect that uncommenting the yield Promise.resolve() which give [1,0], but instead it gives [].

[yelouafi]

@axelson Sorry for the delay. Haven't been able to look at this. Could you open a new issue with your last comment?

[axelson]

@yelouafi Published as issue #413