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

Correct way to close an eventChannel on saga termination #940

Closed
nico1510 opened this issue Apr 27, 2017 · 9 comments
Closed

Correct way to close an eventChannel on saga termination #940

nico1510 opened this issue Apr 27, 2017 · 9 comments
Labels

Comments

@nico1510
Copy link

nico1510 commented Apr 27, 2017

I have this simple authSaga which should run during the whole application lifetime:

export default function* authSaga() {
    const authChannel = eventChannel(emit => {
        const unsubscribe = firebase.auth().onAuthStateChanged(
            user => emit({ user }),
            error => emit({ error })
        )
        return unsubscribe;
    });
    try {
        const task = yield takeLatest(authChannel, handleSyncTask);
    } finally {
        authChannel.close();
    }
}

Only as soon as the app (I'm using react-native) is closed, the finally block should be executed. In reality however it is closed immediately. I looked at the implementation of takeLatest and I understand why it does that. Because the outermost task is forked. That's also why implementing takeLatest myself like this works as expected:

export default function* authSaga() {
    const authChannel = eventChannel(emit => {
        const unsubscribe = firebase.auth().onAuthStateChanged(
            user => emit({ user }),
            error => emit({ error })
        )
        return unsubscribe;
    });
    try {
        let lastTask;
        while (true) {
            const { user, error } = yield take(authChannel);
            if (lastTask) yield cancel(lastTask);
            lastTask = yield fork(handleSyncTask, user, error);
        }
    } finally {
        authChannel.close();
    }
}

The docs however state that The parent will terminate after all launched tasks terminate so I didn't expect the finally block to be executed before the saga terminates. Is there some other clever way to close the channel as soon as the saga terminates ?

@nico1510 nico1510 changed the title Correct way to close an eventChannel Correct way to close an eventChannel on saga termination Apr 27, 2017
@Andarist
Copy link
Member

I think its the best solution, its simple + explicit. There are 2 ways of closing the channel:

  • one used by you, so just calling channel.close() and its intended mostly for such finally blocks like yours
  • emitting special END (exported by the redux-saga) value from the subscriber function, not for use case as its for closing the channel from inside and you need to close from the outside

The docs however state that The parent will terminate after all launched tasks terminate so I didn't expect the finally block to be executed before the saga terminates.

The latter snippet satisfies your requirements, right? If so then everything is correct. If not - I dont exactly see how now, please describe the usage more so I can try to help :)

@nico1510
Copy link
Author

I got it working using this code:

export default function* authSaga() {
    const authChannel = eventChannel(emit => {
        const unsubscribe = firebase.auth().onAuthStateChanged(
            user => emit({ user }),
            error => emit({ error })
        )
        return unsubscribe;
    });
    try {
        yield takeLatest(authChannel, handleSyncTask);
    } finally {
        if (yield cancelled()) authChannel.close();
    }
}

So I'm blocking due to the cancelled effect in the finally block. I didn't think this was necessary because I thought the finally block would only be executed once all forked tasks from the saga terminate which is never since I'm using takeLatest which internally uses a while(true) loop. So basically I only have a problem understanding the library which is why I will close the issue now. Maybe you could still try to clear up my misunderstanding here ? 😀

@Andarist
Copy link
Member

That seems not right, cancelled is not a blocking effect. I think your second snippet in the thread post was fine, or Ive missed some requirement.

takeLatest will return a task descriptor to your saga so the saga can go on and I think it will immediately meet ur finally block and close the channel.

@nico1510
Copy link
Author

nico1510 commented Apr 29, 2017

Yes the second snippet was fine. I didn't want to use it since it looks like it's reimplementing takeLatest so I tried to use takeLatest instead.
However it seems like it immediately hits the finally block even though the saga still has an unterminated task (the one returned from takeLatest). That's what's surprising to me.

@Andarist
Copy link
Member

Its because finally block isnt anything special here. Itd just old plain try/catch/finally situation with exception that finally might be called when ur task terminates. You would have to block on something for it to work cause otherwise ur saga gets resumed with task descriptor as the result of yielding takeLatest and goes on right into ur finally block cause u have wrapped it with try like this.

This seems though like a legitimate use case for join() as self-join. Need to think about this more. Would propose you to just join(task) after yielding takeLatest

@nico1510
Copy link
Author

nico1510 commented May 1, 2017

Thanks @Andarist using join works too. I think I will go with the 'reimplementation' of takeLatest though since it makes it more transparent what's happening.

@James-E-Adams
Copy link

Apologies to revive a closed issue but I was having a similar issue. The channel in previous function calls wasn't getting closed since takeLatest was stopping the channel.close() from being called.
I solved it using @nico1510 's second code block, but I was wondering how you would do it using join?

Also, would it be possible to handle these kinds of situations within the takeLatest implementation? Seems to be an issue that would occur quite frequently and cause memory leaks etc because you're potentially cancelling a task before clean up.

Unless it is implied that you should be doing cleanup within a finally block if you plan on using takeLatest.

@Andarist
Copy link
Member

Andarist commented Jan 9, 2018

The correct approach is imgo the one mentioned here.

but I was wondering how you would do it using join?

Could you give an example you have in mind?

Also, would it be possible to handle these kinds of situations within the takeLatest implementation?

Not rly, you probably want to create a channel before hand and pass it to takeLatest handler - but for the takeLatest its just an argument, it can be on any position and of any type. Detecting channels would be too magical and also probably not something that everyone would like.

@gi
Copy link

gi commented Mar 26, 2019

This is outdated, but I would like a comment on this implementation.

function* saga() {
  // main saga
  yield spawn(watchAuth);
}

function* watchAuth() {
  const channel = eventChannel((emit) => {
    auth.listen((action) => emit(action));
    return () => {
      // unsubscribe
    }
  });

  try {
    const task = yield takeLatest(channel, onAuthAction);
    yield join(task);
  }
  finally {
    channel.close();
  }
}

Since, takeLatest returns a forked task, any finally block would immediately execute, and a check on cancelled would be pointless:

  try {
    yield takeLatest(channel, onAuthAction); // returns immediately
  }
  finally {
    if (yield cancelled()) { // never true
      channel.close(); // never executed
    }
  }

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

No branches or pull requests

4 participants