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

Switch to using Set for Subscription teardowns #6400

Closed
benlesh opened this issue May 12, 2021 · 1 comment · Fixed by #6401
Closed

Switch to using Set for Subscription teardowns #6400

benlesh opened this issue May 12, 2021 · 1 comment · Fixed by #6401
Assignees
Labels
8.x Issues and PRs for version 8.x

Comments

@benlesh
Copy link
Member

benlesh commented May 12, 2021

Related to this issue here: #5638

  1. Using an array of teardowns in our Subscriptions isn't ideal, performance-wise, for removing subscriptions.
  2. Using a Set, rather than an Array would more closely mimic the behavior of most other common event registries, like EventTarget. Consider the following:
function handler() {
   console.log('handled');
}

const target = new EventTarget();
target.addEventListener('unsubscribe', handler);
target.addEventListener('unsubscribe', handler);
target.addEventListener('unsubscribe', handler);

target.dispatchEvent(new Event('unsubscribe')); // Only fires `handler` once!

target.removeEventListener('unsubscribe', handler);

target.dispatchEvent(new Event('unsubscribe')); // handler not fired (as it was removed)

This is in drastic contrast to our Subscription implementation, where the same subscription may be registered n times (which doesn't make sense, because it can only be torn down once), and the same function can also be registered n times, where it will execute n times.

Now, switching to using a Set internally would be a breaking change, as it would change how people can register and remove functions, more than anything. It's doubtful that it would be a breaking change for Subscriptions, because unsubscribe is idempotent.

Therefore we will not be able to do this until version 8.x.

@benlesh benlesh added 8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings labels May 12, 2021
@benlesh benlesh self-assigned this May 12, 2021
benlesh added a commit to benlesh/rxjs that referenced this issue May 12, 2021
- Adding the same instance of a teardown more than once is the same as adding it once
- Removing a teardown with `Subscription.prototype.remove` will remove it much faster

BREAKING CHANGE: Adding the same function instance to a subscription as a teardown multiple times will now result in that function being executed only once on teardown. This brings us inline with the behavior of EventTarget, and also makes removing teardowns faster. The workaround is to make sure you are adding a new function instance to the Subscription each time if you need the same effect.

Resolves ReactiveX#6400
@benlesh
Copy link
Member Author

benlesh commented May 19, 2021

Core team meeting: Approved for next major.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Issues and PRs for version 8.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant