New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

difference between watchers and forked watchers #760

Closed
anthonychung14 opened this Issue Jan 12, 2017 · 23 comments

Comments

Projects
None yet
7 participants
@anthonychung14
Copy link

anthonychung14 commented Jan 12, 2017

Just curious if there was a functional difference between a watcher and a forked watcher

function* testSaga() {
    const anotherChannel = yield actionChannel('another', buffers.expanding(5))
    
    yield [
        watchAnother(anotherChannel),
        fork(watchAnother(anotherChannel)),
        takeLatest('action', irrelevantSaga)
    ]
}

function* watchAnother() {
    while (true) {
        const action = yield take(anotherChannel);
        yield call(delay, 200)
        const data = 'bar'
        yield call(api, data)
    }
}

I have no idea how they'd behave if I ran both, but for my current purposes, it's one or the other.

My hypothesis is that if watchAnother is running in parallel, it's not truly blocking saga mw from catching any other dispatched actions. However, because watchAnother does use "take/call" (I understand these to be blocking), I think it's possible that actions are missed

Forking this process so the below happens seems appropriate, but would appreciate input!

function* testSaga() {
    const anotherChannel = yield actionChannel('another', buffers.expanding(5))

    yield fork(watchAnother(anotherChannel))
    yield takeLatest('action', irrelevantSaga)
}

@Andarist Andarist added the question label Jan 13, 2017

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 13, 2017

Not much of a difference between those 2. You can yield iterators, and you can even yield promises. But the more standarized approach is to yield those 2 wrapped with call or fork (depending on the needs).

Although if you yield fork(generator) you will get back a task descriptor which is way more easier to cancel with yield cancel(taskDescriptor) than plain iterator. Similar APIs might be introduced in the future which will be way easier to apply to the wrapper effect than on the bare metal structures like iterators. Thinking for example about proposing and implementing restart

@Scheduling
do you face some scheduling problem or are you just worried that they may be some there in rare cases?

@fzaninotto

This comment has been minimized.

Copy link

fzaninotto commented Jan 18, 2017

My 2 cents about this: the current code and documentation isn't consistent on how to start several sagas in parallel from a root saga. This brings a lot of confusion about The Right Way To Do It.

In Beginner Tutorial, yield [childSaga()]:

// single entry point to start all Sagas at once
export default function* rootSaga() {
  yield [
    helloSaga(),
    watchIncrementAsync()
  ]
}

In Shopping cart example, yield [fork(childSaga)]:

export default function* root() {
  yield [
    fork(getAllProducts),
    fork(watchGetProducts),
    fork(watchCheckout)
  ]
}

In async example, yield fork(childSaga):

export default function* root() {
  yield fork(startup)
  yield fork(nextRedditChange)
  yield fork(invalidateReddit)
}

What's the advised way and why?

@fzaninotto

This comment has been minimized.

Copy link

fzaninotto commented Jan 18, 2017

Found #178 on the same subject

@granmoe

This comment has been minimized.

Copy link
Contributor

granmoe commented Jan 18, 2017

@fzaninotto Also related: #644

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Jan 18, 2017

Also this comment is related - #737 (comment)

Keep on mind that helper effects - takeLatest, takeEvery, throttle

import { takeEvery } from 'redux-saga/effects'
//...
yield takeEvery(ACTION_A, worker)

are completely the same thing as

import { takeEvery } from 'redux-saga'
import { fork } from 'redux-saga/effects'
//...
yield fork(takeEvery, ACTION_A, worker)

although the latter is deprecated at the moment.

Im saying this only to point out that takeEvery etc in this case returns a task descriptor, cause its wrapped by a fork internally automatically for you, so what I've written there applies directly to your question (2nd vs 3rd option)

As to yielding iterators directly - although possible (in the same way it is possible to directly yield a promise) it is rather advised to yield them wrapped by a call or fork (depending on the needs). It obviously creates some overhead as more stuff needs to be handled internally, but the cost is minimal and it allow you to easily injecting 'resolved' values of the effects in your tests and bunch of other things.

@fzaninotto

This comment has been minimized.

Copy link

fzaninotto commented Jan 19, 2017

Actually, advice spread across issue comments don't help. Is it possible to make the code and documentation consistent on that matter?

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 19, 2017

@fzaninotto unfortunatly it's like asking for JS std to recommand a prefered way to iterate over an array, while there are usecases for forEach, for, map, iterator...

We can't really recommend a way to start root sagas however we can certainly explain better the effect. I think it's probably worth creating a dedicated documentation page.

your examples

1)

// single entry point to start all Sagas at once
export default function* rootSaga() {
  yield [
    saga1(),
    saga2(),
    saga3(),
  ]
}

Here the 3 sagas will be run in parallel. The root saga will block until the 3 sagas complete. If one of the 3 fail, the error will be propagated to the root saga which will be killed, which will also kill the other 2 saga

2)

export default function* root() {
  yield [
    fork(saga1),
    fork(saga2),
    fork(saga3)
  ]
}

The only difference I see here is that this time the yield effect will not block because forking is non-blocking, thus the root saga will reach the end but the 3 childs will remain alive. Error behavior is the same as 1)

3)

export default function* root() {
  yield fork(saga1)
  yield fork(saga2)
  yield fork(saga3)
}

I don't see any difference in behavior from 2)


better examples

The problem with forking is that if any of the root saga fails, then the root saga will be killed, and the other sub sagas will also be killed because their parent got killed. In practice this means that your whole app may become unusable (if it relies heavily on sagas) just because of a minor saga error so it's not really good.

4)

export default function* root() {
  yield spawn(saga1)
  yield spawn(saga2)
  yield spawn(saga3)
}

This time, if an error occur in saga1, it will not make root, saga2 and saga3 get killed so only a part of your app stops working in case of error. Somehow this can also be very problematic because the saga1 might be killed due to an error like a failing http request that you didn't catch properly, making the whole feature covered by saga1 unavailable for the app lifetime.

5)

@granmoe has suggested the following way to start sagas in: #570

function* rootSaga () {

  const sagas = [
    saga1,
    saga2,
    saga3,
  ]; 

  yield sagas.map(saga =>
    spawn(function* () {
      while (true) {
        try {
          yield call(saga)
        } catch (e) {
          console.log(e)
        }
      }
    })
  )

}

This time, if any of the 3 sagas had an error, it would be automatically restarted. This may, or not, be the desired behavior according to your app.

6)

Here's how I handle sagas in my own app:

const makeRestartable = (saga) => {
  return function* () {
    yield spawn(function* () {
      while (true) {
        try {
          yield call(saga);
          console.error("unexpected root saga termination. The root sagas are supposed to be sagas that live during the whole app lifetime!",saga);
        } catch (e) {
          console.error("Saga error, the saga will be restarted",e);
        }
        yield delay(1000); // Avoid infinite failures blocking app TODO use backoff retry policy...
      }
    })
  };
};

const rootSagas = [
  domain1saga,
  domain2saga,
  domain3saga,
].map(makeRestartable);

export default function* root() {
  yield rootSagas.map(saga => call(saga));
}

I'm using a saga HOC to add error handling to the root sagas. In my app, all root sagas are never supposed to terminate but should block, and if there are errors they should be automatically restarted.

Restarting synchronously can, in my experience, lead to infinite loops (if the saga fails everytime you try to restart it) so I added a hacky delay for now to prevent this issue.

You mentionned different domains in your app so this pattern seems appropriate to your usecase where each domain should somehow have its own root saga.


If this is helpful I can write something like that in the doc.

Note that I didn't mention runSaga. You can easily start root sagas through this tool, and it will behave like spawned sagas

See also this SO answer

@fzaninotto

This comment has been minimized.

Copy link

fzaninotto commented Jan 19, 2017

Well, I'm confused even more.

  • you say you can't make redux-saga doc and code consistent, but I don't understand why
  • you say in example 1) the root saga blocks, but since most of the underlying watches use takeEvery, which implicitly fork in 0.14 if I'm not mistaken, then it's not blocking.
  • example 1 seems to have serious drawbacks because of the blocking risk, yet it's what used in the first tutorial in the documentation - the one developers like me will refer to when composing sagas.
  • example 4 uses spawn, which is not the default way to make non-blocking calls according to the documentation (it's fork). I tried to understand the difference between spawn and fork by reading the related doc, and my brain melted.
  • examples 5 and 6 add to the confusion: are they better than the others? Should we forget about all the previous examples and use these? I'm really looking for a simple answer.
  • thinking that users will understand the intricacies of these choices makes redux a bit too elitist IMO. I'm a long time ES6 developer, and I still don't understand what I'm supposed to use in my use case. You'd better choose the best method, and use it all around the doc and say "that's the only way you should do", without explaining how it works under the hood. Much better Developer Experience!

Sorry to take your time on this, but I'm developing an open-source based on saga (admin-on-rest), and I struggle to explain to my users how they should compose their sagas and why.

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 19, 2017

you say you can't make redux-saga doc and code consistent, but I don't understand why

Code and doc can be consistent, and we certainly all want that, but it does not mean that there's a "best way" to start root sagas, just that we can explain the different options.

I'm not either officially from redux-saga team, and everybody can contribute to the doc if something is not clear enough.

you say in example 1) the root saga blocks, but since most of the underlying watches use takeEvery, which implicitly fork in 0.14 if I'm not mistaken, then it's not blocking.

The root saga blocks if any subtask blocks. If all subtasks are not blocking then the root saga will terminate as well because there's nothing to wait for.

example 4 uses spawn, which is not the default way to make non-blocking calls according to the documentation (it's fork). I tried to understand the difference between spawn and fork by reading the related doc, and my brain melted.

Forked tasks will be killed when the parent is killed, while spawned tasks will be kept alive (and you will have to cancel them yourself on errors if you don't want them to keep running)

spawned sagas are a bit like new root sagas that you can start directly from deeper levels.

examples 5 and 6 add to the confusion: are they better than the others? Should we forget about all the previous examples and use these? I'm really looking for a simple answer.

IMHO they are for root sagas, but it's only my opinion and my usecase. And this only applied for root sagas because generally on deeper level you might want errors to propagate.

You might be interested to read as well: #570

thinking that users will understand the intricacies of these choices makes redux a bit too elitist IMO. I'm a long time ES6 developer, and I still don't understand what I'm supposed to use in my use case. You'd better choose the best method, and use it all around the doc and say "that's the only way you should do", without explaining how it works under the hood. Much better Developer Experience!

Redux-saga is probably not yet mature enough and the doc could certainly be better. We are all figuring this out together but as we are iterating progressively according to usecases we find, it becomes clearer over time.

It's true that a lot of information is currently hidden in github comments, and at some point we should probably make the doc better including the most useful comments.

Feel free to apply some techniques mentionned in comments, and give feedback so we figure out together what to recommend to the users

@fzaninotto

This comment has been minimized.

Copy link

fzaninotto commented Jan 19, 2017

Thanks for taking the time to explain, I appreciate.

I'm not either officially from redux-saga team

Then maybe @yelouafi or @Andarist could provide insight, too?

everybody can contribute to the doc if something is not clear enough

I'd love to, I just don't know what to write because I don't understand the rationale behind these gazillion choices.

The root saga blocks if any subtask blocks

So I understand that example 1) should be discouraged, because if a subsaga isn't forked, then the developer exposes themselves to problems. Should we update the tutorial accordingly?

spawned sagas are a bit like new root sagas

My understanding is that they are for advanced usage, so this shouldn't be the main recommendation, right ?

All in all, am I right to think that example 3 is safe to use in most cases, simple to explain, and should be the syntax to encourage?

Redux-saga is probably not yet mature enough and the doc could certainly be better.

I think the code is great but the documentation is clearly lagging in behind. And documentation is at least as important as code for open-source projects. Again, I'd love to help if there was a Definitive Way to do things.

fzaninotto added a commit to marmelab/react-admin that referenced this issue Jan 19, 2017

@anthonychung14

This comment has been minimized.

Copy link
Author

anthonychung14 commented Jan 19, 2017

@slorber great example and recommendation to use spawn at the top-level. It's nice to see real use-cases for spawn.

Can you explain this statement?

Restarting synchronously can, in my experience, lead to infinite loops (if the saga fails everytime you try to restart it) so I added a hacky delay for now to prevent this issue.

  1. How does delay prevent this issue if a saga fails each time on restart?
  2. Rather than yield a delay, can we just yield PUT{ error: true } to an error-reducer? (or yield to an error-catching saga)
@jedrichards

This comment has been minimized.

Copy link

jedrichards commented Jan 20, 2017

I must admit I'm really struggling to feel confident about my redux-saga architecture at the moment too.

It feels like redux-saga is providing some extremely powerful but also very low-level tools at the moment. Something like a set of primitives for composing these async side effect descriptions. Is it fair to say there could be a role for helper libraries to sit on top of redux-saga and provide "blessed" ways to perform the most idiomatic approaches to common use cases?

In any case, is the following a fair description of the most common use-case for redux-saga (at least for developers that are new to it):

  • A need to compose a set of "watchers" within a file into one saga for exporting
  • Then compose those exported watcher sagas into one final root saga
  • Each watcher mainly just watches for one redux action, and then executes a corresponding worker saga
  • All watchers should run in parallel and not block each other
  • Uncaught errors in sagas below the root should not explode the whole app

I'm sure there's a world of more possibilities than that, but it seems like the first step for many? Basically copying the functionality of redux-thunk but with the great testability that comes with sagas?

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 20, 2017

@anthonychung14 try running something like that and you'll see that the main thread stays in Redux-saga forever, freezing your browser tab:

function* alwaysFailingSaga() {
  throw new Error("boom");
}

function* rootSaga() {
      while (true) {
        try {
          yield call(alwaysFailingSaga);
        } catch (e) {
          console.error("Saga error",e);
        }
      }
    }

Yielding a little delay between restarts make room for the browser to execute other things as well.
This is not a production problem, more related to developer experience when you have a big bug in one of your saga you may not like it to freeze your browser. It's very unlikely that you don't notice this bug locally and you ship it to production.

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 20, 2017

@jedrichards yes redux-saga is an generator runtime interpreter with addons to work nicely with redux, but it's low level and does not really provide any recommendations on how you manage your sagas over time, it's just a toolbox.

We've already discussed in the past the creation of an extra repository for useful addons and patterns (including takeLatest/takeEvery/throttle/debounce/error handling/bootstrapping etc) and it's likely someday someone will do that.

@fzaninotto I don't think spawn is only for advanced usage, it's kinda important for the sagas startup usecases like this one and you might need it in other cases too

For me 1) 2) 3) should be avoided for app startup unless you catch carefully errors in each subsaga and do not let them propagate to parent who'll terminate and kill everything. Using spawn or runSaga seems safer to me but it only isolate the failure to a branch of your system and won't handle potential restart that's why I'm using 6 which is more resilient.

fzaninotto added a commit to marmelab/react-admin that referenced this issue Jan 20, 2017

@anthonychung14

This comment has been minimized.

Copy link
Author

anthonychung14 commented Jan 27, 2017

@slorber I tried using the restart strategy you mentioned for sagas, but found that it didn't make them anymore resilient.

A saga failing crashes from a forked saga that failed an api call.

I found that none of my sagas restarted :\

Any idea as to why?

Additionally, is the 1 second truly necessary?

would yield call(delay, 0) work just as well? I'm assuming you're calling the delay just to get another "tick" in the event loop

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 28, 2017

@anthonychung14 I don't know what you tried so I can't say. You should be able to add logs to debug why your saga does not restart. In my 6th example there are logs that might help you understand what is happening.

About the 1sec delay, it's just a simple restart strategy. If you want something more advanced I'd recommend looking at what actor systems use as supervision strategy

Delayed restarts with the BackoffSupervisor pattern
Provided as a built-in pattern the akka.pattern.BackoffSupervisor implements the so-called exponential backoff supervision strategy, starting a child actor again when it fails, each time with a growing time delay between restarts.
This pattern is useful when the started actor fails [1] because some external resource is not available, and we need to give it some time to start-up again. One of the prime examples when this is useful is when a PersistentActor fails (by stopping) with a persistence failure - which indicates that the database may be down or overloaded, in such situations it makes most sense to give it a little bit of time to recover before the peristent actor is started.

@granmoe

This comment has been minimized.

Copy link
Contributor

granmoe commented Jan 28, 2017

@slorber Why a 1 second delay specifically instead of just clearing the stack with setTimeout? And how would you handle sagas that throw synchronously?

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 28, 2017

Don't focus on 1sec it's a random value I pick because I didn't have time to implement something better. The only thing to remember here is that if there's no delay between restart attempts, then you might have a saga that continuously tries to restart itself and freeze the current tick. It probably works with 0sec delay too as long as you let some space for other work to happen.

@granmoe

This comment has been minimized.

Copy link
Contributor

granmoe commented Jan 28, 2017

@slorber If a saga throws synchronously, your approach restarts it forever: http://www.webpackbin.com/41eSzkUwM

Is that why you mentioned that a more robust "backoff strategy" would be needed? I'm just nitpicking because I'd like to use the best solution here in my own apps. I know this is something that should only ever be able to happen during development (like if you make a typo).

@slorber

This comment has been minimized.

Copy link
Contributor

slorber commented Jan 29, 2017

yes @granmoe I want it to try to restart it forever. But I want to give the UI thread some time between restarts to avoid freezing the UI, that's all. There's no best solution, just a solution that works for your usecase. Just try your webpackBin without the delay and you'll immediately understand that it's painful DX to have to kill your chrome tab and reopen it in case of typo

@granmoe

This comment has been minimized.

Copy link
Contributor

granmoe commented Jan 29, 2017

@slorber Oh, I see. Yep, I'm aware of that problem :P I had the same issue in our app, but ended up with another (not ideal) solution:

yield sagas.map(saga =>
    spawn(function* () {
      let isSyncError = false
      while (!isSyncError) {
        isSyncError = true
        try {
          setTimeout(() => isSyncError = false)
          yield call(saga)
          break
        } catch (e) {
          if (isSyncError) {
            throw new Error(saga.name + ' was terminated because it threw an exception on startup.' + e.stack)
          }
          yield put(actions.setError(e.message))
        }
      }
    })
  )
@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Oct 8, 2017

This thread got a little bit stale and also last comments were a littlbe bit off-topic, so I'm closing it. If you need to discuss this further, please just reply back here.

@Andarist Andarist closed this Oct 8, 2017

Granddevv added a commit to Granddevv/React-Admin-panel-restful-apis- that referenced this issue Jan 15, 2018

coderbag added a commit to coderbag/admin-on-rest that referenced this issue Jan 20, 2018

@lydell

This comment has been minimized.

Copy link

lydell commented Jan 25, 2018

As of 2018-01-25 the beginner tutorial says:

export default function* rootSaga() {
  yield all([
    helloSaga(),
    watchIncrementAsync()
  ])
}

As a beginner, I’m going to go with that until I run into problems.

It would be awesome if there was a section in the docs discussing different ways of implementing the root saga, mentioning pros and cons of each. Also mention what multiple calls to sagaMiddleware.run would mean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment