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

Observable.ignoreElements - optimize #2915

Merged
merged 1 commit into from
Apr 27, 2015

Conversation

davidmoten
Copy link
Collaborator

As suggested by @akarnokd, instead of filter(alwaysFalse) use a custom operator and a singleton instance of it.

Also snuck in the removal of an unused import from OperatorTakeLastOneTest.java.


@Override
public Subscriber<? super T> call(final Subscriber<? super T> child) {
return new Subscriber<T>(child) {
Copy link
Member

Choose a reason for hiding this comment

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

This will forward the child's requests and will hang because the child doesn't receive an element and nobody is requesting more. I'd instead write this:

Subscriber<T> result = new Subscribeer<T>() { ... }
child.add(result);
return result;

@davidmoten
Copy link
Collaborator Author

Ah thanks. Now I understand the difference between those two approaches to connecting the subscribers. Simple really. I might add that bit of info to the javadoc on the Subscriber constructor I used in another PR, I was thinking of adding bit more doco to Subscriber anyway because it gets stepped through a lot when you debug and it's a bit terse (confusing variable names and stuff).

@davidmoten
Copy link
Collaborator Author

changes made including a unit test of the request problem (doesn't hang by the way, just terminates early, without processing all upstream).

@akarnokd
Copy link
Member

Great, thanks!

akarnokd added a commit that referenced this pull request Apr 27, 2015
@akarnokd akarnokd merged commit 51f497d into ReactiveX:1.x Apr 27, 2015
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants