-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fixed OutOfMemoryError with CPU scheduler in recursive mode. #643
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
Closed
akarnokd
wants to merge
1
commit into
ReactiveX:master
from
akarnokd:RecursiveSchedulingSubscrChainFix
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
151 changes: 151 additions & 0 deletions
151
rxjava-core/src/main/java/rx/schedulers/ReentrantScheduler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,151 @@ | ||
/** | ||
* Copyright 2013 Netflix, Inc. | ||
* | ||
* 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 rx.schedulers; | ||
|
||
import java.util.concurrent.TimeUnit; | ||
import rx.Scheduler; | ||
import rx.Subscription; | ||
import rx.subscriptions.CompositeSubscription; | ||
import rx.subscriptions.ForwardSubscription; | ||
import rx.subscriptions.SerialSubscription; | ||
import rx.subscriptions.Subscriptions; | ||
import rx.util.functions.Func1; | ||
import rx.util.functions.Func2; | ||
|
||
/** | ||
* Do not re-enter the main scheduler's schedule() method as it will | ||
* unnecessarily chain the subscriptions of every invocation. | ||
*/ | ||
public final class ReentrantScheduler extends Scheduler { | ||
final ReentrantSchedulerHelper scheduler; | ||
final ForwardSubscription scheduleSub; | ||
final ForwardSubscription actionSub; | ||
final CompositeSubscription composite; | ||
|
||
public ReentrantScheduler( | ||
ReentrantSchedulerHelper scheduler, | ||
ForwardSubscription scheduleSub, | ||
ForwardSubscription actionSub, | ||
CompositeSubscription composite) { | ||
this.scheduler = scheduler; | ||
this.scheduleSub = scheduleSub; | ||
this.actionSub = actionSub; | ||
this.composite = composite; | ||
} | ||
|
||
@Override | ||
public <T> Subscription schedule(T state, Func2<? super Scheduler, ? super T, ? extends Subscription> action) { | ||
if (composite.isUnsubscribed()) { | ||
// don't bother scheduling a task which wouldn't run anyway | ||
return Subscriptions.empty(); | ||
} | ||
Subscription before = actionSub.getSubscription(); | ||
final DiscardableAction<T> discardableAction = new DiscardableAction<T>(state, action); | ||
|
||
actionSub.compareExchange(before, discardableAction); | ||
|
||
Runnable r = new Runnable() { | ||
@Override | ||
public void run() { | ||
Subscription sbefore = actionSub.getSubscription(); | ||
Subscription s = discardableAction.call(ReentrantScheduler.this); | ||
actionSub.compareExchange(sbefore, s); | ||
} | ||
}; | ||
|
||
scheduler.scheduleTask(r, scheduleSub); | ||
|
||
return composite; | ||
} | ||
|
||
@Override | ||
public <T> Subscription schedule(T state, Func2<? super Scheduler, ? super T, ? extends Subscription> action, long delayTime, TimeUnit unit) { | ||
if (composite.isUnsubscribed()) { | ||
// don't bother scheduling a task which wouldn't run anyway | ||
return Subscriptions.empty(); | ||
} | ||
|
||
Subscription before = actionSub.getSubscription(); | ||
final DiscardableAction<T> discardableAction = new DiscardableAction<T>(state, action); | ||
actionSub.compareExchange(before, discardableAction); | ||
|
||
Runnable r = new Runnable() { | ||
@Override | ||
public void run() { | ||
Subscription sbefore = actionSub.getSubscription(); | ||
Subscription s = discardableAction.call(ReentrantScheduler.this); | ||
actionSub.compareExchange(sbefore, s); | ||
} | ||
}; | ||
scheduler.scheduleTask(r, scheduleSub, delayTime, unit); | ||
|
||
return composite; | ||
} | ||
|
||
@Override | ||
public <T> Subscription schedulePeriodically(T state, Func2<? super Scheduler, ? super T, ? extends Subscription> action, long initialDelay, long period, TimeUnit unit) { | ||
if (composite.isUnsubscribed()) { | ||
// don't bother scheduling a task which wouldn't run anyway | ||
return Subscriptions.empty(); | ||
} | ||
|
||
Subscription before = actionSub.getSubscription(); | ||
final PeriodicAction<T> periodicAction = new PeriodicAction<T>(state, action); | ||
actionSub.compareExchange(before, periodicAction); | ||
|
||
Runnable r = new Runnable() { | ||
@Override | ||
public void run() { | ||
Subscription sbefore = actionSub.getSubscription(); | ||
Subscription s = periodicAction.call(ReentrantScheduler.this); | ||
actionSub.compareExchange(sbefore, s); | ||
} | ||
}; | ||
scheduler.scheduleTask(r, scheduleSub, initialDelay, period, unit); | ||
|
||
return composite; | ||
} | ||
/** | ||
* An action that calls the underlying function in a periodic environment. | ||
* @param <T> the state value type | ||
*/ | ||
private static final class PeriodicAction<T> implements Subscription, Func1<Scheduler, Subscription> { | ||
final T state; | ||
final Func2<? super Scheduler, ? super T, ? extends Subscription> underlying; | ||
final SerialSubscription ssub; | ||
|
||
public PeriodicAction(T state, Func2<? super Scheduler, ? super T, ? extends Subscription> underlying) { | ||
this.state = state; | ||
this.underlying = underlying; | ||
this.ssub = new SerialSubscription(); | ||
} | ||
|
||
@Override | ||
public Subscription call(Scheduler scheduler) { | ||
if (!ssub.isUnsubscribed()) { | ||
Subscription s = underlying.call(scheduler, state); | ||
ssub.setSubscription(s); | ||
return ssub; | ||
} | ||
return Subscriptions.empty(); | ||
} | ||
|
||
@Override | ||
public void unsubscribe() { | ||
ssub.unsubscribe(); | ||
} | ||
} | ||
} |
54 changes: 54 additions & 0 deletions
54
rxjava-core/src/main/java/rx/schedulers/ReentrantSchedulerHelper.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/** | ||
* Copyright 2013 Netflix, Inc. | ||
* | ||
* 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 rx.schedulers; | ||
|
||
import java.util.concurrent.TimeUnit; | ||
import rx.subscriptions.ForwardSubscription; | ||
|
||
/** | ||
* Simple scheduler API used by the ReentrantScheduler to | ||
* communicate with the actual scheduler implementation. | ||
*/ | ||
public interface ReentrantSchedulerHelper { | ||
/** | ||
* Schedule a task to be run immediately and update the subscription | ||
* describing the schedule. | ||
* @param r the task to run immediately | ||
* @param out the subscription holding the current schedule subscription | ||
*/ | ||
void scheduleTask(Runnable r, ForwardSubscription out); | ||
|
||
/** | ||
* Schedule a task to be run after the delay time and update the subscription | ||
* describing the schedule. | ||
* @param r the task to schedule | ||
* @param out the subscription holding the current schedule subscription | ||
* @param delayTime the time to delay the execution | ||
* @param unit the time unit | ||
*/ | ||
void scheduleTask(Runnable r, ForwardSubscription out, long delayTime, TimeUnit unit); | ||
|
||
/** | ||
* Schedule a task to be run after the delay time and after | ||
* each period, then update the subscription describing the schedule. | ||
* @param r the task to schedule | ||
* @param out the subscription holding the current schedule subscription | ||
* @param initialDelay the initial delay of the schedule | ||
* @param period the between period of the schedule | ||
* @param unit the time unit | ||
*/ | ||
void scheduleTask(Runnable r, ForwardSubscription out, long initialDelay, long period, TimeUnit unit); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This feels wrong that we have a "helper" that schedulers extend from. That implies that the
Scheduler
interface is wrong./cc @headinthebox
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 needed this as the scheduler working with a reentrant scheduler should provide some core scheduling operations without creating chained subscriptions through the standard API. But ExecutorScheduler could implement this privately so it doesn't show up in the signature.
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.
If the default (Executor)Scheduler would schedule the stateless tasks without turning them into a recursive call, there were no need to use a helper: