-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fixed schedule race and task retention with ExecutorScheduler. #2907
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
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 |
---|---|---|
|
@@ -15,21 +15,14 @@ | |
*/ | ||
package rx.schedulers; | ||
|
||
import java.util.concurrent.ConcurrentLinkedQueue; | ||
import java.util.concurrent.Executor; | ||
import java.util.concurrent.Future; | ||
import java.util.concurrent.RejectedExecutionException; | ||
import java.util.concurrent.ScheduledExecutorService; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.*; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; | ||
import rx.Scheduler; | ||
import rx.Subscription; | ||
|
||
import rx.*; | ||
import rx.functions.Action0; | ||
import rx.internal.schedulers.ScheduledAction; | ||
import rx.plugins.RxJavaPlugins; | ||
import rx.subscriptions.CompositeSubscription; | ||
import rx.subscriptions.MultipleAssignmentSubscription; | ||
import rx.subscriptions.Subscriptions; | ||
import rx.subscriptions.*; | ||
|
||
/** | ||
* Scheduler that wraps an Executor instance and establishes the Scheduler contract upon it. | ||
|
@@ -58,12 +51,12 @@ static final class ExecutorSchedulerWorker extends Scheduler.Worker implements R | |
// TODO: use a better performing structure for task tracking | ||
final CompositeSubscription tasks; | ||
// TODO: use MpscLinkedQueue once available | ||
final ConcurrentLinkedQueue<ExecutorAction> queue; | ||
final ConcurrentLinkedQueue<ScheduledAction> queue; | ||
final AtomicInteger wip; | ||
|
||
public ExecutorSchedulerWorker(Executor executor) { | ||
this.executor = executor; | ||
this.queue = new ConcurrentLinkedQueue<ExecutorAction>(); | ||
this.queue = new ConcurrentLinkedQueue<ScheduledAction>(); | ||
this.wip = new AtomicInteger(); | ||
this.tasks = new CompositeSubscription(); | ||
} | ||
|
@@ -73,11 +66,15 @@ public Subscription schedule(Action0 action) { | |
if (isUnsubscribed()) { | ||
return Subscriptions.unsubscribed(); | ||
} | ||
ExecutorAction ea = new ExecutorAction(action, tasks); | ||
ScheduledAction ea = new ScheduledAction(action, tasks); | ||
tasks.add(ea); | ||
queue.offer(ea); | ||
if (wip.getAndIncrement() == 0) { | ||
try { | ||
// note that since we schedule the emission of potentially multiple tasks | ||
// there is no clear way to cancel this schedule from individual tasks | ||
// so even if executor is an ExecutorService, we can't associate the future | ||
// returned by submit() with any particular ScheduledAction | ||
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. This comment is somewhat confusing to me. The "potentially multiple tasks" is just that we're scheduling the "this" reference over and over again isn't it? We are scheduling a single "this" one at a time so that it then drains from the queue to ensure sequential execution. Thus, the scheduling on the 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. This is the scenario: I submit 2 tasks concurrently. The first one will start the schedule with |
||
executor.execute(this); | ||
} catch (RejectedExecutionException t) { | ||
// cleanup if rejected | ||
|
@@ -96,7 +93,10 @@ public Subscription schedule(Action0 action) { | |
@Override | ||
public void run() { | ||
do { | ||
queue.poll().run(); | ||
ScheduledAction sa = queue.poll(); | ||
if (!sa.isUnsubscribed()) { | ||
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. Well that's a sad bug we had sitting in there allowing unsubscribed tasks to still execute! |
||
sa.run(); | ||
} | ||
} while (wip.decrementAndGet() > 0); | ||
} | ||
|
||
|
@@ -115,28 +115,54 @@ public Subscription schedule(final Action0 action, long delayTime, TimeUnit unit | |
service = GenericScheduledExecutorService.getInstance(); | ||
} | ||
|
||
final MultipleAssignmentSubscription first = new MultipleAssignmentSubscription(); | ||
final MultipleAssignmentSubscription mas = new MultipleAssignmentSubscription(); | ||
// tasks.add(mas); // Needs a removal without unsubscription | ||
mas.set(first); | ||
tasks.add(mas); | ||
final Subscription removeMas = Subscriptions.create(new Action0() { | ||
@Override | ||
public void call() { | ||
tasks.remove(mas); | ||
} | ||
}); | ||
|
||
try { | ||
Future<?> f = service.schedule(new Runnable() { | ||
@Override | ||
public void run() { | ||
if (mas.isUnsubscribed()) { | ||
return; | ||
} | ||
mas.set(schedule(action)); | ||
// tasks.delete(mas); // Needs a removal without unsubscription | ||
ScheduledAction ea = new ScheduledAction(new Action0() { | ||
@Override | ||
public void call() { | ||
if (mas.isUnsubscribed()) { | ||
return; | ||
} | ||
}, delayTime, unit); | ||
mas.set(Subscriptions.from(f)); | ||
// schedule the real action untimed | ||
Subscription s2 = schedule(action); | ||
mas.set(s2); | ||
// unless the worker is unsubscribed, we should get a new ScheduledAction | ||
if (s2.getClass() == ScheduledAction.class) { | ||
// when this ScheduledAction completes, we need to remove the | ||
// MAS referencing the whole setup to avoid leaks | ||
((ScheduledAction)s2).add(removeMas); | ||
} | ||
} | ||
}); | ||
// This will make sure if ea.call() gets executed before this line | ||
// we don't override the current task in mas. | ||
first.set(ea); | ||
// we don't need to add ea to tasks because it will be tracked through mas/first | ||
|
||
|
||
try { | ||
Future<?> f = service.schedule(ea, delayTime, unit); | ||
ea.add(f); | ||
} catch (RejectedExecutionException t) { | ||
// report the rejection to plugins | ||
RxJavaPlugins.getInstance().getErrorHandler().handleError(t); | ||
throw t; | ||
} | ||
|
||
return mas; | ||
/* | ||
* This allows cancelling either the delayed schedule or the actual schedule referenced | ||
* by mas and makes sure mas is removed from the tasks composite to avoid leaks. | ||
*/ | ||
return removeMas; | ||
} | ||
|
||
@Override | ||
|
@@ -150,46 +176,4 @@ public void unsubscribe() { | |
} | ||
|
||
} | ||
|
||
/** Runs the actual action and maintains an unsubscription state. */ | ||
static final class ExecutorAction implements Runnable, Subscription { | ||
final Action0 actual; | ||
final CompositeSubscription parent; | ||
volatile int unsubscribed; | ||
static final AtomicIntegerFieldUpdater<ExecutorAction> UNSUBSCRIBED_UPDATER | ||
= AtomicIntegerFieldUpdater.newUpdater(ExecutorAction.class, "unsubscribed"); | ||
|
||
public ExecutorAction(Action0 actual, CompositeSubscription parent) { | ||
this.actual = actual; | ||
this.parent = parent; | ||
} | ||
|
||
@Override | ||
public void run() { | ||
if (isUnsubscribed()) { | ||
return; | ||
} | ||
try { | ||
actual.call(); | ||
} catch (Throwable t) { | ||
RxJavaPlugins.getInstance().getErrorHandler().handleError(t); | ||
Thread thread = Thread.currentThread(); | ||
thread.getUncaughtExceptionHandler().uncaughtException(thread, t); | ||
} finally { | ||
unsubscribe(); | ||
} | ||
} | ||
@Override | ||
public boolean isUnsubscribed() { | ||
return unsubscribed != 0; | ||
} | ||
|
||
@Override | ||
public void unsubscribe() { | ||
if (UNSUBSCRIBED_UPDATER.compareAndSet(this, 0, 1)) { | ||
parent.remove(this); | ||
} | ||
} | ||
|
||
} | ||
} |
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.
Couldn't we include a reference to
queue
in theScheduledAction
so that if it is unsubscribed it can remove itself from the queue?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.
The problem with that is that remove from queue is O(n) and we'd pay it most of the time (once dequeued, remove will traverse the entire queue and not find it) because we can't distinguish between the normal completion calling unsubscribe and external unsubscribe.