Skip to content
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

support cancelling child subroutines when parent routine is cancelled #17

Merged
merged 7 commits into from
Jan 4, 2016

Conversation

aikoven
Copy link
Contributor

@aikoven aikoven commented Dec 25, 2015

No description provided.

@yelouafi
Copy link
Member

thanks @aikoven for your contribution.

For subroutine cancellation I think auto cancellation of child subroutines isn't a good idea. IMO it'd be better to let the developer explicitly have control of when to trigger the cancellation.

function* subtask2() {
    try { ... } 
    catch(err) { 
       doCleanup 
    }
}

function* subtask() {
   const subtask = yield fork(subtask2) 
   ...
    try {
              ...
    } catch(err) {
      needToDoBeforeChildcancellation()
      subtask.cancel() // explicit cancellation of subtasks
    } 
}

function mainTask() {
   const task = yield fork(subtask)
   ...
   task.cancel()
}

(As a side note I think your code doesnt handle the case if a Saga forked more than one subroutine)

I think also we should also throw a more specific error, something like SagaCancellationError. This way we can know if the catch block was triggered by a cancellation or other error

@aikoven
Copy link
Contributor Author

aikoven commented Dec 30, 2015

That's a different case: I covered subroutines that were invoked using call, not fork. I agree that forked subtasks should be cancelled by hand, because they live in separate 'process'. But if called subroutine is in progress when parent routine is cancelled, then there is no way to actually cancel subroutine.

@aikoven
Copy link
Contributor Author

aikoven commented Dec 30, 2015

Also it seems to be nontrivial to make a custom Error extension, see http://stackoverflow.com/questions/1382107/whats-a-good-way-to-extend-error-in-javascript
On the other hand we don't need stack trace for these errors, so we can probably go with just plain class.

@aikoven
Copy link
Contributor Author

aikoven commented Dec 30, 2015

Just noticed that my code actually cancels forked subroutines, not only called. Will fix

@yelouafi
Copy link
Member

But if called subroutine is in progress when parent routine is cancelled, then there is no way to actually cancel subroutine.

Thanks for drawing my attention on this. I was too focused on forked tasks.

There are some more issues i'd like to discuss

1- taking into account that a task can be blocked on multiple calls. for example

function* task() {
  const [r1, r2] = yield [call(subtask1), call(subtask2)]
}

we need to cancel both subtasks if task is cancelled while waiting for the 2 subtasks to complete.

2- making task cancellation declarative. so instead of task.cancel() we write yield cancel(task). I think it makes more sens since fork and join are already declaratives

3- Do you think we should automatically cancel called subtasks that have lost in a race. for example

function* task() {
  ...
   const {timeout} = yield race({
      timeout: call(delay, 1000),
      subtask: call(subtask)
    });
}

if timeout wins the race, should we automatically cancel the call(subtask) ?

@aikoven
Copy link
Contributor Author

aikoven commented Dec 30, 2015

  1. Good point, I will make another commit to fix that
  2. I guess that makes sense too
  3. In my auth example I have following code:
const authLoopTask = yield fork(authorizeLoop, storedToken);

const {signOutAction} = yield race({
  signOutAction: take(SIGN_OUT),
  authLoop: join(authLoopTask)
});

if (signOutAction) {
  authLoopTask.cancel();
  // ...
}

authLoop here is an infinite cycle that ends only when cancelled. I use fork here, not call, but if we did as you propose, it would be shorter:

const {signOutAction} = yield race({
  signOutAction: take(SIGN_OUT),
  authLoop: call(authorizeLoop, storedToken)
});

if (signOutAction) {
  // ...
}

So probably it's a good idea, but I need to think a bit more about this.
Maybe it would be convenient to make promise returned from proc() cancellable and make race cancel every cancellable promise that had lost. Although I don't know if there is a standard for cancellable promises yet. See this discussion: whatwg/fetch#27

@aikoven
Copy link
Contributor Author

aikoven commented Jan 4, 2016

@yelouafi Done.

I decided not to go with cancellable promises because there is no convention and it would result in additional complications with making chained promises cancellable as well.
So I just cancel all subroutine that is still running after race resolves.

@yelouafi
Copy link
Member

yelouafi commented Jan 4, 2016

@aikoven great work! thanks 👍 .

My only doubt is on the automatic race cancellation. we should cancel the cancellable/competing effects. I looked quickly, but it seems like you're cancelling subroutines of the Saga that is running the actual effects.

@yelouafi
Copy link
Member

yelouafi commented Jan 4, 2016

@aikoven
Sorry. It seems like your code is correct after all. If the Saga that is running the actual race advances; all pending subroutines should be cancelled as we're no longer blocked on them.

yelouafi added a commit that referenced this pull request Jan 4, 2016
support cancelling child subroutines when parent routine is cancelled
@yelouafi yelouafi merged commit 6b1eacb into redux-saga:task-cancel Jan 4, 2016
@aikoven
Copy link
Contributor Author

aikoven commented Jan 4, 2016

You are right, my code will go wrong if there was another called subroutine outside of race, e.g.:

yield [
  call(subroutine1),
  race({
    subroutine2: call(subroutine2),
    subroutine3: call(subroutine3),
  })
]

If race finishes before subroutine1, then it will be cancelled

@yelouafi
Copy link
Member

yelouafi commented Jan 4, 2016

You are right, my code will go wrong if there was another called subroutine outside of race, e.g.:

Oops! merged too soon!

@aikoven
Copy link
Contributor Author

aikoven commented Jan 4, 2016

I made a fix. I can make another PR, or you can revert the merge 😀

@yelouafi
Copy link
Member

yelouafi commented Jan 5, 2016

I reverted the merge. Not sure however if this will reactivate the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants