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(buffer): subscribe to source and closingNotifier in proper order #2195

Merged
merged 2 commits into from
Jun 14, 2017
Merged

fix(buffer): subscribe to source and closingNotifier in proper order #2195

merged 2 commits into from
Jun 14, 2017

Conversation

mpodlasin
Copy link
Contributor

Description:

In buffer operator subscribe to source observable first, so that when
closingNotifier emits value, all source values emited before land in buffer

Related issue (if exists):

Closes #1610

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.001%) to 97.675% when pulling 5f3c1bd on Podlas29:buffer-subscription-order into 5f93f81 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Dec 15, 2016

Is this the behavior of RxJS 4? Worth confirming.

@benlesh
Copy link
Member

benlesh commented Dec 15, 2016

It looks like Rx5 and Rx4 currently differ in behavior, even given the same scheduling. So this is something we need to look at and fix.

@mpodlasin
Copy link
Contributor Author

mpodlasin commented Dec 15, 2016

@Blesh
In RxJS 4

const Observable = Rx.Observable;

const xs = Observable.from([0,1,2,3,4,5,6]);

xs.buffer(xs.filter(x => x % 2 === 0))
	.subscribe(x => console.log(x));

results in
[0]
[1, 2]
[3, 4]

After my change rxjs 5 behaves the same (as seen in my unit test).
Do you have any cases where behavior is different?

@mpodlasin
Copy link
Contributor Author

@Blesh bare in mind that this pr will also partially help solve: #1754

@jayphelps
Copy link
Member

jayphelps commented Feb 14, 2017

Needs a BREAKING CHANGE disclaimer in the commit message body so that it will show up in our automatic CHANGELOG.md generation. In it, describe what the current (incorrect) behavior is, then what the new behavior is and why it's being changed. Feel free to be verbose if it helps with clarity.

Copy link
Member

@jayphelps jayphelps left a comment

Choose a reason for hiding this comment

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

Otherwise code looks good. Needs more reviews though since this is a fundamental change.

In buffer operator subscribe to source observable first, so that when
closingNotifier emits value, all source values emited before land in buffer

Closes #1610
BREAKING CHANGE:
When source and closingNotifier fire at the same time, it is expected
that value emitted by source will first land in buffer and then
closingNotifier will close it. Because of reversed subscription order,
closingNotifier emitted first, so source was not able to put value in
buffer before it was closed. Now source is subscribed before closingNotifier,
so if they fire at the same time, source value is put into buffer and then
closingNotifer closes it.
@mpodlasin
Copy link
Contributor Author

@jayphelps done. please let me know if message is ok. ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 97.69% when pulling 425aca5 on Podlas29:buffer-subscription-order into 7839002 on ReactiveX:master.

@jayphelps jayphelps changed the base branch from master to next February 15, 2017 20:02
@jayphelps
Copy link
Member

I changed to base branch aka merge target to next since this is a breaking change.

Still need one more person to review

@coveralls
Copy link

coveralls commented Feb 15, 2017

Coverage Status

Coverage increased (+0.001%) to 97.691% when pulling 15d5ac9 on Podlas29:buffer-subscription-order into a102b3c on ReactiveX:next.

@benlesh benlesh merged commit 41e33f5 into ReactiveX:next Jun 14, 2017
@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.

4 participants