-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(debounceTime): unschedule dangling task on unsubscribe before complete #6464
fix(debounceTime): unschedule dangling task on unsubscribe before complete #6464
Conversation
I think it would be 'more correct' to add the scheduled action - |
e058bdf
to
4c7deff
Compare
didnt notice that this was possible, i pushed a version, where the activeTask subscription is managed within the outer subscriber |
It ought to be possible to test this using the rxjs/spec/schedulers/AnimationFrameScheduler-spec.ts Lines 107 to 110 in 3d251be
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is fine, but this needs a test that's fixed by the change.
4c7deff
to
6103acb
Compare
test added (and verified that it is failing without the fix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
Thank you, @backbone87 |
Description:
Since
debounceTime
is not based ondebounce
anymore (#6049), the "notifier subscription" is not managed within the parent subscription anymore, but scheduled "raw". We need to make sure we dont leave dangling tasks behind.I am not sure how to test this.
edit: this also prevents stopped notifications, if the source errors, because that could also leave dangling tasks behind