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

fix(window): don't track internal window subjects as subscriptions. #1708

Merged
merged 1 commit into from
May 24, 2016

Conversation

trxcllnt
Copy link
Member

Description:
This PR removes behavior in the window operators that tracked the window Subjects as Subscriptions. This could lead to race conditions where the Subscriber unsubscribes while the window operator is still active and attempting to send messages to the windows.

Related issue (if exists):
#1698

@trxcllnt trxcllnt force-pushed the fix-window-subject-subscriptions branch from 64991dd to 98e7d13 Compare May 12, 2016 18:42
@trxcllnt trxcllnt force-pushed the fix-window-subject-subscriptions branch from 98e7d13 to cbee158 Compare May 13, 2016 22:58
window.subscribe();
}).to.throw(Rx.ObjectUnsubscribedError);
}, late);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this test went away. Can you elaborate? Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Blesh now that window's internal Subjects aren't being tracked as if they were Subscriptions, their unsubscribe method is never called when the subscriber disposes. This test attempts to subscribe to one of the window Subjects after the outer window subscription has been disposed, assuming the WindowSubscriber has called unsubscribe on the Subject. This PR invalidates that assumption.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks

@benlesh
Copy link
Member

benlesh commented May 22, 2016

It seems like #1705 resolves #1698. @trxcllnt, can you explain the relationship between #1698 and this PR?

@trxcllnt trxcllnt force-pushed the fix-window-subject-subscriptions branch from cbee158 to f3357b9 Compare May 24, 2016 00:25
@trxcllnt
Copy link
Member Author

@Blesh #1705 resolves #1669 and partially fixed #1698, after which I discovered an unrelated race condition. The window operators would continue to next or complete the internal window Subjects, even if a downstream subscriber unsubscribed in response to a next or complete event. Since the internal Subjects would be disposed at the time the downstream subscriber unsubscribed, attempting to message them again would throw ObjectUnsubscribedErrors. Minimal repro example:

Observable.range(0, 2)
  .windowCount(1).mergeAll()
  .take(1).subscribe();

@benlesh
Copy link
Member

benlesh commented May 24, 2016

Looks good to me, @trxcllnt. :shipit:

@benlesh benlesh merged commit 2a7b001 into ReactiveX:master May 24, 2016
@trxcllnt trxcllnt deleted the fix-window-subject-subscriptions branch June 16, 2016 05:13
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants