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

Unsubscribe a listener inside a listener has side-effects on other listeners #461

Closed
arian opened this issue Aug 11, 2015 · 3 comments
Closed
Labels

Comments

@arian
Copy link
Contributor

arian commented Aug 11, 2015

Unsubscribing a listener inside a listener, will splice the listeners array in the store of createStore, while looping over that array using forEach in dispatch.

For example the following program fails:

import assert from 'assert';
import {createStore} from 'redux';

var store = createStore(() => []);

var countA = 0;
var countB = 0;
var countC = 0;

var unsubA = store.subscribe(() => {
    countA++;
});

var unsubB = store.subscribe(() => {
    countB++;
    unsubB();
});

var unsubC = store.subscribe(() => {
    countC++;
});

store.dispatch({type: 'X'});
store.dispatch({type: 'Y'});

console.log(countA, countB, countC);

assert.equal(countA, 2, 'countA');
assert.equal(countB, 1, 'countB');
assert.equal(countC, 2, 'countC');

countC is 1, but it should be 2.

A fix for this problem would be to copy the array before doing the forEach in the dispatch function, for example:

listeners.slice().forEach(listener => listener());
@gaearon
Copy link
Contributor

gaearon commented Aug 11, 2015

Thanks! Would you please add a failing test? (And maybe you'd like to contribute a fix too? ;-)
The branch for development is breaking-changes-1.0.

@gaearon gaearon added the bug label Aug 11, 2015
arian added a commit to arian/redux that referenced this issue Aug 11, 2015
…the loop.

Unsubscribing during a dispatch would change the listeners array.
However that causes the next listener not to fire.
gaearon added a commit that referenced this issue Aug 11, 2015
Fixes #461 - Copy listeners array, so subscribes can't affect the loop.
@gaearon
Copy link
Contributor

gaearon commented Aug 11, 2015

Fixed by #462.

@gaearon gaearon closed this as completed Aug 11, 2015
acstll pushed a commit to acstll/chopped-redux that referenced this issue Aug 21, 2015
@xogeny
Copy link

xogeny commented Dec 3, 2015

I have a question about this. The idea is the copy the array so subscribes can't affect the loop. But if the first listener unsubscribes a later listener, that won't be handled correctly. The unsubscribed listener will be called one more time after it was unsubscribed.

Is that really the desired behavior? I agree it is a tricky issue and I'm not 100% certain how to address that. But before even trying to create a PR I wanted to take a step back and ask the "big picture" question of "When do we expect an unsubscribe to be applied?". Specifically, how should this case where one listener unsubscribes a later listener? (one of many potential corner cases worth considering)

Although not strictly relevant, I think it might be worth nothing that this is an issue with React because you can get into a situation (this is how I got here, in fact) where a component gets unsubscribed when it is unmounted. But the listener is invoke on more time after the unsubscribe/unmounting and the listener calls setState which leads to a nasty message on the console. I'm trying to avoid that situation. In my case, I want the unsubscribe to be effective immediately because otherwise the listener will be called after the component is unmounted.

Thoughts?

mtiller added a commit to xogeny/vada that referenced this issue Dec 3, 2015
I ran into a case where the subscription listener was called
after the unsubscribe.  This resulted in setState being called
on unmounted components.  So I introduced an extra flag to
avoid this.  But this is pretty complicated.  A simpler solution
would be to have this handled inside redux which is why I
commented on this issue: reduxjs/redux#461
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

3 participants