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

unsubscribing immediately in a listener #1204

Closed
wants to merge 8 commits into
base: master
from

Conversation

3 participants
@slorber
Contributor

slorber commented Jan 6, 2016

It passes npm run check

See https://github.com/rackt/redux/issues/1180

@tappleby

This comment has been minimized.

Contributor

tappleby commented Jan 6, 2016

If this gets merged would impact redux-batched-subscribe, it would be nice if I didnt need to duplicate the execute listener logic.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Jan 6, 2016

@tappleby Any ideas how to solve it in a way that wouldn't impact you?

@slorber

This comment has been minimized.

Contributor

slorber commented Jan 6, 2016

@tappleby unfurtunatly I think it would be hard to have the exact same behavior with your store enhancer because of its own nature.

If you look at the test case I created:

  it('removes listeners immediately when unsubscribe is called', () => {
    const store = createStore(reducers.todos)

    const unsubscribeHandles = []
    const doUnsubscribeAll = () => unsubscribeHandles.forEach(unsubscribe => unsubscribe() )

    const listener1 = expect.createSpy(() => {})
    unsubscribeHandles.push(store.subscribe(listener1))
    const listener2 = expect.createSpy(() => {})
    unsubscribeHandles.push(store.subscribe(listener2))

    const listener3 = () => doUnsubscribeAll()
    unsubscribeHandles.push(store.subscribe(listener3))

    const listener4 = expect.createSpy(() => {})
    unsubscribeHandles.push(store.subscribe(listener4))
    const listener5 = expect.createSpy(() => {})
    unsubscribeHandles.push(store.subscribe(listener5))

    store.dispatch(unknownAction())
    expect(listener1.calls.length).toBe(1)
    expect(listener2.calls.length).toBe(1)
    // listener3 is the one removing the others so obviously it got executed
    expect(listener4.calls.length).toBe(0)
    expect(listener5.calls.length).toBe(0)
  })

By using your store enhanced I guess none of the listeners will be called at all, because they are always all unsubscribed even before you start to iterate on them.

I'm not sure how my commit could affect that behavior in any way but maybe you can provide a test case that pass before my commit and fails afterward?

@tappleby

This comment has been minimized.

Contributor

tappleby commented Jan 7, 2016

@slorber the enhancer would still be prone to the same issue; it executes the same code, just at a different point. Part of the issue is since dispatch is the only real hook into redux core, I had to duplicate the subscribe logic (plus override dispatch + subscribe).

@gaearon without getting into the higher level issues with extending redux core and having hooks into different parts of the dispatch cycle, I think the easiest solution is to move the subscribe logic into a util that I could consume in redux-batched-subscribe.

also, is it necessary to have different code paths depending on where we are in the dispatch cycle? It seems like things could be simplified if we only GC the listeners array once every dispatch (after notifying subscribers):

function createEmitter() {
  var listeners = []
  var hasUnsubscribedInListener = false

  function subscribe(listener) {
    listeners.push(listener)
    var isSubscribed = true
    return function unsubscribe() {
      var index = listeners.indexOf(listener)

      if ( isSubscribed && index >= 0 ) {
        isSubscribed = false
        hasUnsubscribedInListener = true
        listeners[index] = undefined // Do not change the array, because we might be iterating it!
      }
    }
  }

  function notify() {
    try {
      // No need to copy the array. If a listener gets unsubscribed during dispatch
      // we just replace it by "undefined" to avoid messing up with the iteration
      listeners.forEach(listener => {
        if ( typeof listener !== 'undefined' ) {
          listener()
        }
      })
    } finally {
      if ( hasUnsubscribedInListener ) {
        listeners = listeners.filter(listener => typeof listener !== 'undefined')
        hasUnsubscribedInListener = false
      }
    }
  }

  return { subscribe, notify }
}

@slorber slorber changed the title from This is an attempt to solve the problem of unsubscribing immediately in a listener to unsubscribing immediately in a listener Jan 13, 2016

listeners.slice().forEach(listener => listener())
listeners.slice().forEach(listener => {
// Check if subscription still exists (#1180)
if (listeners.includes(listener)) {

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

This would assume user has ES6 polyfill (we don't currently depend on it). Also AFAIK includes can potentially be O(n) so it's bad to call in a loop.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Jan 27, 2016

Thank you for investigating this. Please see my comment in https://github.com/rackt/redux/issues/1180#issuecomment-175324172.

@gaearon gaearon closed this Jan 27, 2016

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