-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
2.x: add limit() to limit both item count and request amount #5655
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
/** | ||
* Copyright (c) 2016-present, RxJava Contributors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in | ||
* compliance with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is | ||
* distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See | ||
* the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
package io.reactivex.internal.operators.flowable; | ||
|
||
import java.util.concurrent.atomic.AtomicLong; | ||
|
||
import org.reactivestreams.*; | ||
|
||
import io.reactivex.*; | ||
import io.reactivex.annotations.Experimental; | ||
import io.reactivex.internal.subscriptions.*; | ||
import io.reactivex.plugins.RxJavaPlugins; | ||
|
||
/** | ||
* Limits both the total request amount and items received from the upstream. | ||
* | ||
* @param <T> the source and output value type | ||
* @since 2.1.6 - experimental | ||
*/ | ||
@Experimental | ||
public final class FlowableLimit<T> extends AbstractFlowableWithUpstream<T, T> { | ||
|
||
final long n; | ||
|
||
public FlowableLimit(Flowable<T> source, long n) { | ||
super(source); | ||
this.n = n; | ||
} | ||
|
||
@Override | ||
protected void subscribeActual(Subscriber<? super T> s) { | ||
source.subscribe(new LimitSubscriber<T>(s, n)); | ||
} | ||
|
||
static final class LimitSubscriber<T> | ||
extends AtomicLong | ||
implements FlowableSubscriber<T>, Subscription { | ||
|
||
private static final long serialVersionUID = 2288246011222124525L; | ||
|
||
final Subscriber<? super T> actual; | ||
|
||
long remaining; | ||
|
||
Subscription upstream; | ||
|
||
LimitSubscriber(Subscriber<? super T> actual, long remaining) { | ||
this.actual = actual; | ||
this.remaining = remaining; | ||
lazySet(remaining); | ||
} | ||
|
||
@Override | ||
public void onSubscribe(Subscription s) { | ||
if (SubscriptionHelper.validate(this.upstream, s)) { | ||
if (remaining == 0L) { | ||
s.cancel(); | ||
EmptySubscription.complete(actual); | ||
} else { | ||
this.upstream = s; | ||
actual.onSubscribe(this); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void onNext(T t) { | ||
long r = remaining; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the point of local There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I avoid re-reading fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, it's not volatile, so it should be normally in CPU register/cache, but yeah local var has slightly more chances to be there Although local var creates indirection between read/writes, so overall I think it's neither win nor loss in performance :) There is interesting question on SO, but answers seem to only compare speed of access to the field when it's already initialized, not taking overall overhead of additional variable https://stackoverflow.com/questions/21613098/java-local-vs-instance-variable-access-speed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JIT may load it into a register but not everything runs on HotSpot. |
||
if (r > 0L) { | ||
remaining = --r; | ||
actual.onNext(t); | ||
if (r == 0L) { | ||
upstream.cancel(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about this: if (r > 0L) {
remaining = --r;
final boolean completed = r == 0L;
if (completed) {
upstream.cancel();
}
actual.onNext(t);
if (completed) {
actual.onComplete();
}
} This way we cancel upstream before delivering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, current implementations of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or it could trigger an interrupted thread and fail code in the downstream unexpectedly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! But doesn't it mean that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's way less likely to happen. There were issues around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, although I think probability is the same here, |
||
actual.onComplete(); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void onError(Throwable t) { | ||
if (remaining > 0L) { | ||
remaining = 0L; | ||
actual.onError(t); | ||
} else { | ||
RxJavaPlugins.onError(t); | ||
} | ||
} | ||
|
||
@Override | ||
public void onComplete() { | ||
if (remaining > 0L) { | ||
remaining = 0L; | ||
actual.onComplete(); | ||
} | ||
} | ||
|
||
@Override | ||
public void request(long n) { | ||
if (SubscriptionHelper.validate(n)) { | ||
for (;;) { | ||
long r = get(); | ||
if (r == 0L) { | ||
break; | ||
} | ||
long toRequest; | ||
if (r <= n) { | ||
toRequest = r; | ||
} else { | ||
toRequest = n; | ||
} | ||
long u = r - toRequest; | ||
if (compareAndSet(r, u)) { | ||
upstream.request(toRequest); | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void cancel() { | ||
upstream.cancel(); | ||
} | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this strict version of
take()
, but thinking about API design I'm afraid that this additional operator could add unnecessary confusion for the users.Maybe an overload of
take()
with boolean flag that controls "strictness" of the upstream requests would work? (or something better if you have it on your mind)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly.