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

Operator: forEach #147

Merged
merged 6 commits into from
Feb 15, 2013
Merged

Conversation

benjchristensen
Copy link
Member

Issue #45

Related to Pull #131

@benjchristensen
Copy link
Member Author

Updated in another commit to bring inline with the MSDN docs.

Unit tests that show the behavior:

    @Test
    public void testForEach() {
        Observable.create(new AsyncObservable()).forEach({ result -> a.received(result)});
        verify(a, times(1)).received(1);
        verify(a, times(1)).received(2);
        verify(a, times(1)).received(3);
    }

    @Test
    public void testForEachWithError() {
        try {
            Observable.create(new AsyncObservable()).forEach({ result -> throw new RuntimeException('err')});
            fail("we expect an exception to be thrown");
        }catch(Exception e) {
            // do nothing as we expect this
        }
        verify(a, times(0)).received(1);
        verify(a, times(0)).received(2);
        verify(a, times(0)).received(3);
    }

}catch(Exception e) {
// do nothing as we expect this
}
verify(a, times(0)).received(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Verifications could be removed since there are no usages of a in the forEach closure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Submitting a commit to remove those now.

@benjchristensen
Copy link
Member Author

Thanks @mairbek for the review.

@dcapwell @jhusain or anyone else have comments on this before I merge?

I'll wait a few hours for feedback before merging and doing a release with this last batch of changes.

@dcapwell
Copy link

Looks good to me. Only comment i would make is in the InterruptedException catch. Since not throwing a InterruptedException but Runtime, it might be good to also add Thread.getCurrentThread().interrupt() to set the interrupt flag again.

    try {
        latch.await();  
    } catch (InterruptedException e) {  
        Thread.getCurrentThread().interrupt();
        throw new RuntimeException("Interrupted while waiting for subscription to complete.", e);
 }

@benjchristensen
Copy link
Member Author

Interesting point, didn't even think of that as I figured the RuntimeException would be sufficient to cause the caller to behave appropriately, but they could theoretically swallow RuntimeExceptions and continue in a while loop check for isInterrupted() which could be lost in this scenario.

Making that change now.

benjchristensen added a commit that referenced this pull request Feb 15, 2013
@benjchristensen benjchristensen merged commit 481f948 into ReactiveX:master Feb 15, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants