Inconsistent Blocking Actions about redux-saga HOT 13 CLOSED

I think there are interesting ideas here, and if we can find ways to enforce that sagas are always "safe", it will be great. As it stands, most examples in saga's own documentation contain race conditions. I think it is safe to say if the docs for a library contain race conditions, so will real world programmes that use that library.

One possible alternative solution, instead of:

import { put, subscribe } from 'redux-saga'
// sagas/index.js

const incrementAsync = subscribe(['INCREMENT_ASYNC'], function* (take) {

  while(true) {

    // wait for each INCREMENT_ASYNC action  
    const nextAction = yield take()

    // delay is a sample function
    // return a Promise that resolves after (ms) milliseconds
    yield delay(1000)

    // dispatch INCREMENT_COUNTER
    yield put( increment() )
  }

});

export default [incrementAsync]

You explicitly specify up-font which action types you are interested in, which are then buffered until you take them. I don't think this removes any of the flexibility you would want, but it prevents the current footgun.

from redux-saga.

@ForbesLindesay the new release 0.8.0 introduced the helper functions takeEvery and takeLatest which offers a safer way for new users. The helpers are introduced early in the docs (README, beginner tutorial and basics section)

take is not introduced until the advanced section And Immediately followed by a section on Non blocking calls. This section illustrates the pitfalls of using take with call, and shows the right way to do non blocking calls when using take to implement complex flows.

I also started refactoring the examples in the repo to use the helpers.

Do you think this would be sufficient to prevent the mentioned race issues ? New users are exposed only to the helper functions. And advanced users are warned about the use of take with blocking calls.

Also you may find here an interesting case of how 'missing' the actions allows implementing some useful patterns like throttling and debouncing.

from redux-saga.

but it misses any extra actions fired in that 1 second delay.

Yes you're right (perhaps not the best example to start with).

To express parallelism, we use fork, for example

import { take, put, fork } from 'redux-saga'

// worker saga
function* incrementAsync() {
  yield delay(1000)
  yield put( increment() )
}

// watcher saga
function* watchIncrementAsync() {
  while(true) {
    const nextAction = yield take(INCREMENT_ASYNC)
    yield fork(incrementAsync)
  }
}

The watcher saga can also be made reusable as illustrated in your createSaga example

import { take, put, fork } from 'redux-saga'

function* watch(actionType, workerSaga) {
  while(true) {
    const nextAction = yield take(actionType)
    yield fork(workerSaga)
  }
}

// start incrementAsync on each INCREMENT_ASYNC
watch(INCREMENT_ASYNC, incrementAsync)

which mimics somewhat the callback behavior

We can also express concurrency between parallel tasks, if for example we want each new incoming INCREMENT_ASYNC action to cancel any ongoing async increment and keep only the latest one (like flatmapLatest in Observables)

function* incrementAsync() { ... }

// watcher saga
function* watchIncrementAsync() {
  let task
  while(true) {
    const nextAction = yield take(INCREMENT_ASYNC)
    // cancel task if pending
    if(task)
      yield cancel(task)

    task = yield fork(incrementAsync)
  }
}

Note this behavior can also be made reusbale

function* incrementAsync() { ... }

// watcher saga
function* watchLatest(actionType, workerSaga) {
  let task
  while(true) {
    const nextAction = yield take(actionType)
    // cancel task if pending
    if(task)
      yield cancel(task)

    task = yield fork(workerSaga)
  }
}

watchLatest(INCREMENT_ASYNC, incrementAsync)

You don't know in advance when or if a saga will attempt to take a given action type, so you are forced to buffer all actions indefinitely.

One of the main reason I avoided buffering is because of the related memory issues as you mentioned, this was already discussed (see this comment).

Event buffering, via channels, has the benefit of being more predictable. Especially for case involving inter-saga communication. The time or the order (ie who started first) of which inter-communicating sagas has started or dispatched doesn't matter (for example see this pin pong example from the js-csp site)

but I don't think you should be putting state inside your sagas. Keep your state in the store (updated via reducers).

Actually, there 2 different opinions about this.

@slorber (who introduced me to the Saga concept). thinks it's not right for Sagas to be coupled with state shape and only depend on the event log (Redux actions). see #8

EDIT: fixed example code

from redux-saga.

As a side note, for the above examples to work; fork has to be truly non blocking, ie has to resume immediately/synchronously. Otherwise it may miss other actions dispatched synchronously from components/Redux middlewares. with the promise-based version of the lib, fork doesn't resolve until the next microtask which make it loose events happened in-between.

I tried first to fix this by keeping the actions for some time for the current event queue (using setImmediate / requestAnimationFrame as a timeout to discard the action later) But some other issues persisted; for example when a saga does yield fork(otherSaga). otherSaga takes some time to start (in the next microtask) which makes it miss its startup actions (buffering wont work because the saga hasn't started yet and we can't give it any action [each saga has its own action buffer and shared action buffer is not possible because we must then keep all actions all the time for future sagas]);

The solution seemed to be the parent saga passing the current action queue to all its forked/called child sagas but it doesn't work; The solution began to look more and more hacky/complicated so I decided to stop right here. and took the other more radical way: using callbacks mainly because they don't have the latency problem of microtasks. And because it make it possible for the library to express true non-blocking semantics (ie non-blocking means the saga resumes immediately)

from redux-saga.

If you want the flexibility of things like watchLatest, you could implement them outside of saga:

function* mySaga(action) {
  // ... business logic here ...
}
const mySagaWrapped = createSaga(MY_ACTION_NAME, mySaga);
let lastSaga = null;
export default function (action) {
  if (lastSaga) lastSaga.cancel();
  return lastSaga = mySagaWrapped(action);
};

But I can understand how that might start to get confusing. With this in mind, I would propose codifying the separation. Define two types "Watchers" and "Sagas".

• Watchers should only be able to yield take, fork and cancel. (maybe also put if that can be made consistently non-blocking).
• Sagas should be able to access everything except take.

At the very least, your examples should all religiously enforce this separation, or you will just leave users with these subtle race conditions.

As for promises, you could have any non-blocking calls (e.g. take, fork and cancel) return plain values, and delay etc. can return promises.

from redux-saga.

At the very least, your examples should all religiously enforce this separation, or you will just leave users with these subtle race conditions.

There are still some cases which can not be trivially described by the watcher/worker pattern; in some complex flows: a worker may itself forks other worker at some moments. The point is that the model gives enough flexiblity to implement from simple to complex flows; some time we may need a behavior different than watch, watchLatest (eg. put the results of requests according to their dispatch order)

But Perhaps you're right; maybe the lib should provide helper functions for the more common use cases (like watch/wachLatest) and use them in the repo examples. This could act as a guard against a bunch of future issues;

As for promises, you could have any non-blocking calls (e.g. take, fork and cancel) return plain values, and delay etc. can return promises.

When I said I shifted away from promises, I didn't mean the library doesn't support yielding promises. I was talking about the internal implementation;

redux-saga is not like libs like co; take, call, fork and all other effects actually all returns plain object; even if you do yield call(delay, 1000); call(delay) doesn't call the delay function. It just creates a plain object description like {type: 'call', func: delay, args: [1000]} and the execution/interpretation is performed by the middleware. It makes possible testing the generator easily without executing any real service call, simply iterate over the generator and examines the yielded values.

the core function that drives the generator internally takes all yielded effects (take, call ...); execute them then wraps all results automatically in promises; using promise simplifies the generator plumbing because it exposes an unified interface, and also parallelism and race could be implemented directly on top of Promise.all/Promise.race. The problem is that using promise chaining to drive the generator causes the issues I mentioned; So I just got rid of the promise wrapping and used callback notification internally: when results of effects are simple values I notify the generator to resume immediately and when the result is a promise I notify when the promise resolve/reject.

from redux-saga.

or you will just leave users with these subtle race conditions.

If I understand. Your point is that the lib actually provides a double-edged sword

from redux-saga.

Yes, using this lib as it currently stands would mean that a tiny mistake can result in subtle race conditions that are the kind of thing that take weeks to track down in large apps.

from redux-saga.

One example of using take with delay where (I think) you dont run into this kind of issue

while(true) {
  let interval = 5;
  yield put({ type: FETCH });

  const { stop, start } = yield race({
    delay: call(delay, interval),
     start: take(Constants.FETCH_START),
     stop: take(Constants.FETCH_STOP),
  });

  if (stop) {
    break;
   } else if (start) {
    interval = start.payload;
   }
}

Maybe its that the worker sagas cant yield a take at the "top level"?

from redux-saga.

Yes, that's an interesting example. Based on this, it's OK to call any blocking method, as long as it's racing against a take method. You could support validating that still I suppose.

from redux-saga.

@ForbesLindesay I understand your point but in practice using redux-saga you soon understand that you should take great care of this kind of race problems.
I think the library is a low-level building block. Maybe it's not yet the most easy to use, but as @yelouafi said you can easily build meaningful abstractions on top of it like watch/watchLatest.
If you think some devs will make mistake, then make sure you understand redux-saga yourself and give them useful and easy to understand abstractions to work with, or teach them these subtilities

If your usage is just to replace redux-thunk to redux-saga for api call, watch/watchLatest interface is enough in most cases, but redux-saga is not just a lib to handle api calls.

In my app when we click on an username we load that username profile page fullscreen.
If we click very fast on 2 distincts usernames, then I want to ensure that we will always display the last clicked username profile, no matter the time the requests took. watchLatest is more appropriate in that case, while it may not be for async counter. If I wanted to express that with redux-thunk I would probably have to make some kind of cancellable promise in action creators (and I'm not really sure where I would store it? global variable?)

from redux-saga.

@ForbesLindesay I was thinking of creating a redux-saga-contrib repo which provides higher level abstractions (like watch). I'm writing a step by step tutorial and I find it a little hard to write correct examples without introducing too much concepts at once (fork, call, blocking/non blocking)

Thinking if we've such a higher level lib, then we can make the project more accessible. And newcomers can use safe abstractions provided by default. Advanced users can still use the low level functions to implement non trivial flows (and of course Advanced users should know when to make blocking and non blocking calls)

About race conditions, it's true that using call carelessely exposes users to race conditions. But simple callbacks have also their own kind of race conditions: If a new Redux user for example uses redux-thunk just as it is to handle AJAX requests, there are also subtle race conditions here (out of order responses although this is less serious than missing actions).

While I agree that we should provide safe abstractions to be used by new users. IMO the main goal shouldn't be to hide race conditions. Instead we need the developers to be aware of them when they're specifying an async control flow. The developer has always to ask

• Do I need a simple merge like the one provided by callbacks ?
• Do I need to ignore concurrent actions (e.g. infinite scroll) ?
• Do I need to take only the latest action ?
• Do I need to queue concurrent actions ?

from redux-saga.

Sorry here is the link to the comment

#105 (comment)

from redux-saga.