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 for overlapping windows. #2897

Merged
merged 1 commit into from
Apr 21, 2015
Merged

Conversation

alexwen
Copy link

@alexwen alexwen commented Apr 20, 2015

Source was emitting t multiple times while holding queue.

Fixes #2896

@davidmoten
Copy link
Collaborator

Could you add the unit test from your gist as well?

@davidmoten
Copy link
Collaborator

Ah, someone else's gist. Still, as the unit test is there may as well chuck it in.

@alexwen
Copy link
Author

alexwen commented Apr 21, 2015

Thanks for the feedback. Is it kosher to do a sleep in the test? Its the
only way I've been able to get the windows to overlap and reproduce the
condition.

Also, it doesn't seem too reliable to depend upon sleep.

@akarnokd
Copy link
Member

Good catch! An unit test would be great, but you don't really need concurrency: reentrancy will do just fine:

    @Test
    public void testWindowNoDuplication() {
        final PublishSubject<Integer> source = PublishSubject.create();
        final TestSubscriber<Integer> tsw = new TestSubscriber<Integer>() {
            boolean once;
            @Override
            public void onNext(Integer t) {
                if (!once) {
                    once = true;
                    source.onNext(2);
                }
                super.onNext(t);
            }
        };
        TestSubscriber<Observable<Integer>> ts = new TestSubscriber<Observable<Integer>>() {
            @Override
            public void onNext(Observable<Integer> t) {
                t.subscribe(tsw);
                super.onNext(t);
            }
        };
        source.window(new Func0<Observable<Object>>() {
            @Override
            public Observable<Object> call() {
                return Observable.never();
            }
        }).subscribe(ts);

        source.onNext(1);
        source.onCompleted();

        assertEquals(1, ts.getOnNextEvents().size());
        assertEquals(Arrays.asList(1, 2), tsw.getOnNextEvents());
    }

@akarnokd akarnokd added the Bug label Apr 21, 2015
@alexwen
Copy link
Author

alexwen commented Apr 21, 2015

Thanks @akarnokd - great test!

@akarnokd
Copy link
Member

Thanks!

akarnokd added a commit that referenced this pull request Apr 21, 2015
@akarnokd akarnokd merged commit 7147a8d into ReactiveX:1.x Apr 21, 2015
@benjchristensen
Copy link
Member

Should I push a patch release with this today?

@akarnokd
Copy link
Member

A verification on #2878 would be great. After that, the patch release can commence.

@benjchristensen benjchristensen mentioned this pull request Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants