Skip to content

Commit 4152834

Browse files
committed
Don't destroy the task private storage until task is destroyed since
other threads/tasks which have a reference to the task might need to access parts of it even if the task itself is completed. Radar-Id: rdar://problem/88093007
1 parent 6285226 commit 4152834

File tree

3 files changed

+24
-16
lines changed

3 files changed

+24
-16
lines changed

include/swift/ABI/Task.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ class AsyncTask : public Job {
325325

326326
/// Flag that this task is now completed. This normally does not do anything
327327
/// but can be used to locally insert logging.
328-
void flagAsCompleted();
328+
void flagAsDestroyed();
329329

330330
/// Check whether this task has been cancelled.
331331
/// Checking this is, of course, inherently race-prone on its own.

stdlib/public/Concurrency/Task.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ static void destroyJob(SWIFT_CONTEXT HeapObject *obj) {
330330
}
331331

332332
AsyncTask::~AsyncTask() {
333-
flagAsCompleted();
333+
flagAsDestroyed();
334334

335335
// For a future, destroy the result.
336336
if (isFuture()) {
@@ -372,7 +372,7 @@ static void destroyTask(SWIFT_CONTEXT HeapObject *obj) {
372372
// the task-local allocator. There's actually nothing else to clean up
373373
// here.
374374

375-
SWIFT_TASK_DEBUG_LOG("destroy task %p", task);
375+
SWIFT_TASK_DEBUG_LOG("Destroyed task %p", task);
376376
free(task);
377377
}
378378

stdlib/public/Concurrency/TaskPrivate.h

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -287,11 +287,9 @@ class alignas(2 * sizeof(void*)) ActiveTaskStatus {
287287
/// or is completed.
288288
IsEnqueued = 0x1000,
289289

290-
#ifndef NDEBUG
291290
/// Task has been completed. This is purely used to enable an assertion
292291
/// that the task is completed when we destroy it.
293292
IsComplete = 0x2000,
294-
#endif
295293
};
296294

297295
// Note: this structure is mirrored by ActiveTaskStatusWithEscalation and
@@ -409,7 +407,6 @@ class alignas(2 * sizeof(void*)) ActiveTaskStatus {
409407
#endif
410408
}
411409

412-
#ifndef NDEBUG
413410
bool isComplete() const {
414411
return Flags & IsComplete;
415412
}
@@ -421,7 +418,6 @@ class alignas(2 * sizeof(void*)) ActiveTaskStatus {
421418
return ActiveTaskStatus(Record, Flags | IsComplete);
422419
#endif
423420
}
424-
#endif
425421

426422
/// Is there a lock on the linked list of status records?
427423
bool isStatusRecordLocked() const { return Flags & IsStatusRecordLocked; }
@@ -596,9 +592,7 @@ struct AsyncTask::PrivateStorage {
596592
auto newStatus = oldStatus.withRunning(false);
597593
newStatus = newStatus.withoutStoredPriorityEscalation();
598594
newStatus = newStatus.withoutEnqueued();
599-
#ifndef NDEBUG
600595
newStatus = newStatus.withComplete();
601-
#endif
602596

603597
// This can fail since the task can still get concurrently cancelled or
604598
// escalated.
@@ -615,10 +609,22 @@ struct AsyncTask::PrivateStorage {
615609
}
616610
}
617611

618-
// Destroy and deallocate any remaining task local items.
619-
// We need to do this before we destroy the task local deallocator.
612+
// Destroy and deallocate any remaining task local items since the task is
613+
// completed. We need to do this before we destroy the task local
614+
// deallocator.
620615
Local.destroy(task);
621616

617+
// Don't destroy the task private storage as a whole since others which have
618+
// a reference to the task might still need to access the ActiveTaskStatus.
619+
// It is destroyed when task is destroyed
620+
}
621+
622+
// Destroy the opaque storage of the task
623+
void destroy() {
624+
#if NDEBUG
625+
auto oldStatus = task->_private()._status().load(std::memory_order_relaxed);
626+
assert(oldStatus.isComplete());
627+
#endif
622628
this->~PrivateStorage();
623629
}
624630

@@ -653,11 +659,13 @@ inline void AsyncTask::OpaquePrivateStorage::initializeWithSlab(
653659
JobPriority basePri, void *slab, size_t slabCapacity) {
654660
::new (this) PrivateStorage(basePri, slab, slabCapacity);
655661
}
662+
656663
inline void AsyncTask::OpaquePrivateStorage::complete(AsyncTask *task) {
657664
get().complete(task);
658665
}
666+
659667
inline void AsyncTask::OpaquePrivateStorage::destroy() {
660-
// nothing else to do
668+
get().destroy();
661669
}
662670

663671
inline AsyncTask::PrivateStorage &AsyncTask::_private() {
@@ -822,10 +830,10 @@ inline void AsyncTask::flagAsSuspended() {
822830
}
823831

824832
// READ ME: This is not a dead function! Do not remove it! This is a function
825-
// that can be used when debugging locally to instrument when a task actually is
826-
// dealloced.
827-
inline void AsyncTask::flagAsCompleted() {
828-
SWIFT_TASK_DEBUG_LOG("task completed %p", this);
833+
// that can be used when debugging locally to instrument when a task
834+
// actually is dealloced.
835+
inline void AsyncTask::flagAsDestroyed() {
836+
SWIFT_TASK_DEBUG_LOG("task destroyed %p", this);
829837
}
830838

831839
inline void AsyncTask::localValuePush(const HeapObject *key,

0 commit comments

Comments
 (0)