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

RxJava2 Observable.zip taking Iterable and a zipper is broken #4524

Closed
dynaxis opened this issue Sep 10, 2016 · 12 comments
Closed

RxJava2 Observable.zip taking Iterable and a zipper is broken #4524

dynaxis opened this issue Sep 10, 2016 · 12 comments

Comments

@dynaxis
Copy link

dynaxis commented Sep 10, 2016

http://reactivex.io/RxJava/2.x/javadoc/io/reactivex/Observable.html#zip-java.lang.Iterable-io.reactivex.functions.Function-

The zipper signature is Function<? super T[]>, ? extends R> zipper, where use of T[] breaks it. Just invoke it with non Object T, and a ClassCastException is thrown.

I checked with Single and Flowable, they have different signatures for the comparable zip operators. They use Object[] where T[] is used for Observable, which are also consistent with the 1.x zip operator.

I think this is just a bug in the signature.

@akarnokd
Copy link
Member

Totally possible. Can you post a small unit test that fails?

@dynaxis
Copy link
Author

dynaxis commented Sep 10, 2016

    @Test
    public void zipIterableOfObservables() {
        List<Observable<Integer>> observables =
                new ArrayList<Observable<Integer>>();
        observables.add(Observable.just(1, 2, 3));
        observables.add(Observable.just(1, 2, 3));

        Observable.zip(observables, new Function<Integer[], Integer>() {
            @Override
            public Integer apply(Integer[] o) throws Exception {
                int sum = 0;
                for (int i : o) {
                    sum += i;
                }
                return sum;
            }
        }).test().assertResult(2, 4, 6);
    }

Please note that if we use lambda as a zipper, it eventually generates what the above test has.

@dynaxis
Copy link
Author

dynaxis commented Sep 10, 2016

FYI, the following works and it's what is generated with the 1.x signature:

    @Test
    public void zipIterableOfObservables() {
        List<Observable<Integer>> observables =
                new ArrayList<Observable<Integer>>();
        observables.add(Observable.just(1, 2, 3));
        observables.add(Observable.just(1, 2, 3));

        Observable.zip(observables, new Function<Object[], Object>() {
            @Override
            public Object apply(Object[] o) throws Exception {
                int sum = 0;
                for (Object i : o) {
                    sum += (Integer) i;
                }
                return sum;
            }
        }).test().assertResult(2, 4, 6);
    }

@akarnokd akarnokd added this to the 2.0 RC 3 milestone Sep 10, 2016
@akarnokd
Copy link
Member

Thanks. The underlying problem is that we can't do new T[n] and such lambdas cast the bridge Object apply(Object[]) argument to Integer[] which fails since zip and combineLatest use Object[] internally.

Currently I can't write a PR. @vanniktech could you change all ? super T[] signatures back to ? super Object[] and add unit tests like above to verify there is no ClassCastException in a PR?

@vanniktech
Copy link
Collaborator

Sure 👍

@akarnokd
Copy link
Member

Closing via #4525.

@dynaxis
Copy link
Author

dynaxis commented Sep 23, 2016

BTW, why use array instead of List there? Any reason? If List is used, then at least it seems safe with generics, doesn't it?

@akarnokd
Copy link
Member

Using array has reduced allocation cost and less indirection.

@hafs-r
Copy link

hafs-r commented Dec 31, 2017

I've Singles of different types to run with Single.zip, and I'm getting ClassCastException as SingleZipIterable. Is it wrong zipping Singles of different type together?

@akarnokd
Copy link
Member

You have to be careful which index you cast back to what type.

@hafs-r
Copy link

hafs-r commented Jan 2, 2018

My Single.zip returns io.reactivex.internal.operators.single.SingleZipIterable but I'm expecting a Single, any Advice?

@akarnokd
Copy link
Member

akarnokd commented Jan 2, 2018

Please provide a standalone unit test that demonstrates your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants