Fix Window by Count Unsubscribe Behavior #1829
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Split the unit tests up to match the implementation files. Add unit tests for ReactiveX#1546 to OperatorWindowWithSizeTest
- ReactiveX#1546 - This also fixes the fact that the overlapping window overload was not propagating unsubscribe before. A new unit test caught that.
benjchristensen
added a commit
that referenced
this pull request
Nov 6, 2014
Fix Window by Count Unsubscribe Behavior
| @Override | ||
| public void call() { | ||
| // if no window we unsubscribe up otherwise wait until window ends | ||
| if(window == null) { |
There was a problem hiding this comment.
I'm not sure window has proper Thread visibility here.
There was a problem hiding this comment.
It's better to mark parentSubscription final
There was a problem hiding this comment.
I guess over an observeOn on boundary this could become an issue (and if someone took a reference and put it in another thread and called it).
There was a problem hiding this comment.
Here is a possible fix: #1833
My eyes are glazing over though so it may be completely wrong :-)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #1546 where unsubscribes would occur prematurely for non-overlapping
window(size)and would not unsubscribe at all for overlappingwindow(size, skip).It also reorganizes the
windowunit tests which were all combined in one file when the implementations are split across 4 files. This is now easier to understand and use.