Skip to content

Commit efa62e7

Browse files
committed
[Concurrency] Fix crash from calling fromTaskExecutorPreference after enqueueing job.
After we've enqueued a job, another thread may run it and destroy it. Don't try to get the job's task executor preference when we try to schedule it. Instead, get the task executor preference before enqueueing the job, then use that preference when scheduling if necessary. Since getting the executor preference is potentially somewhat expensive (we need to search the status records for an executor preference record), only do this if the pre-compare-and-swap states look like they'll need it. rdar://136281920
1 parent 2947510 commit efa62e7

File tree

1 file changed

+24
-6
lines changed

1 file changed

+24
-6
lines changed

stdlib/public/Concurrency/Actor.cpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1355,20 +1355,31 @@ void DefaultActorImpl::enqueue(Job *job, JobPriority priority) {
13551355
}
13561356
}
13571357

1358+
// Fetch the task executor from the job for later use. This can be somewhat
1359+
// expensive, so only do it if we're likely to need it. The conditions here
1360+
// match the conditions of the if statements below which use `taskExecutor`.
1361+
TaskExecutorRef taskExecutor;
1362+
bool needsScheduling = !oldState.isScheduled() && newState.isScheduled();
1363+
bool needsStealer =
1364+
oldState.getMaxPriority() != newState.getMaxPriority() &&
1365+
newState.isRunning();
1366+
if (needsScheduling || needsStealer)
1367+
taskExecutor = TaskExecutorRef::fromTaskExecutorPreference(job);
1368+
13581369
// This needs to be a store release so that we also publish the contents of
13591370
// the new Job we are adding to the atomic job queue. Pairs with consume
13601371
// in drainOne.
13611372
if (_status().compare_exchange_weak(oldState, newState,
13621373
/* success */ std::memory_order_release,
13631374
/* failure */ std::memory_order_relaxed)) {
1375+
// NOTE: `job` is off limits after this point, as another thread might run
1376+
// and destroy it now that it's enqueued.
1377+
13641378
traceActorStateTransition(this, oldState, newState, distributedActorIsRemote);
13651379

13661380
if (!oldState.isScheduled() && newState.isScheduled()) {
13671381
// We took responsibility to schedule the actor for the first time. See
13681382
// also ownership rule (1)
1369-
TaskExecutorRef taskExecutor =
1370-
TaskExecutorRef::fromTaskExecutorPreference(job);
1371-
13721383
return scheduleActorProcessJob(newState.getMaxPriority(), taskExecutor);
13731384
}
13741385

@@ -1389,8 +1400,6 @@ void DefaultActorImpl::enqueue(Job *job, JobPriority priority) {
13891400
this, newState.getMaxPriority());
13901401
swift_retain(this);
13911402

1392-
TaskExecutorRef taskExecutor =
1393-
TaskExecutorRef::fromTaskExecutorPreference(job);
13941403
scheduleActorProcessJob(newState.getMaxPriority(), taskExecutor);
13951404
}
13961405
}
@@ -1446,9 +1455,19 @@ void DefaultActorImpl::enqueueStealer(Job *job, JobPriority priority) {
14461455
if (oldState == newState)
14471456
return;
14481457

1458+
// Fetch the task executor from the job for later use. This can be somewhat
1459+
// expensive, so only do it if we're likely to need it. The conditions here
1460+
// match the conditions of the if statements below which use `taskExecutor`.
1461+
TaskExecutorRef taskExecutor;
1462+
if (!newState.isRunning() && newState.isScheduled())
1463+
taskExecutor = TaskExecutorRef::fromTaskExecutorPreference(job);
1464+
14491465
if (_status().compare_exchange_weak(oldState, newState,
14501466
/* success */ std::memory_order_relaxed,
14511467
/* failure */ std::memory_order_relaxed)) {
1468+
// NOTE: `job` is off limits after this point, as another thread might run
1469+
// and destroy it now that it's enqueued.
1470+
14521471
traceActorStateTransition(this, oldState, newState, distributedActorIsRemote);
14531472
#if SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION
14541473
if (newState.isRunning()) {
@@ -1466,7 +1485,6 @@ void DefaultActorImpl::enqueueStealer(Job *job, JobPriority priority) {
14661485
"[Override] Scheduling a stealer for actor %p at %#x priority",
14671486
this, newState.getMaxPriority());
14681487
swift_retain(this);
1469-
auto taskExecutor = TaskExecutorRef::fromTaskExecutorPreference(job);
14701488
scheduleActorProcessJob(newState.getMaxPriority(), taskExecutor);
14711489
}
14721490
#endif

0 commit comments

Comments
 (0)