Skip to content

Commit 750263f

Browse files
ktosoktoso
committed
[Discarding] Don't leak retained "first error" task when retaining it
1 parent 019a84a commit 750263f

File tree

10 files changed

+335
-67
lines changed

10 files changed

+335
-67
lines changed

‎include/swift/ABI/MetadataValues.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2334,6 +2334,7 @@ class TaskCreateFlags : public FlagSet<size_t> {
23342334
Task_InheritContext = 11,
23352335
Task_EnqueueJob = 12,
23362336
Task_AddPendingGroupTaskUnconditionally = 13,
2337+
Task_IsDiscardingTask = 14,
23372338
};
23382339

23392340
explicit constexpr TaskCreateFlags(size_t bits) : FlagSet(bits) {}
@@ -2360,6 +2361,9 @@ class TaskCreateFlags : public FlagSet<size_t> {
23602361
FLAGSET_DEFINE_FLAG_ACCESSORS(Task_AddPendingGroupTaskUnconditionally,
23612362
addPendingGroupTaskUnconditionally,
23622363
setAddPendingGroupTaskUnconditionally)
2364+
FLAGSET_DEFINE_FLAG_ACCESSORS(Task_IsDiscardingTask,
2365+
isDiscardingTask,
2366+
setIsDiscardingTask)
23632367
};
23642368

23652369
/// Flags for schedulable jobs.

‎stdlib/public/Concurrency/DiscardingTaskGroup.swift

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -171,21 +171,21 @@ public struct DiscardingTaskGroup {
171171
#if SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY
172172
@available(*, unavailable, message: "Unavailable in task-to-thread concurrency model", renamed: "addTask(operation:)")
173173
#endif
174-
public mutating func addTask<DiscardedResult>(
174+
public mutating func addTask(
175175
priority: TaskPriority? = nil,
176-
operation: __owned @Sendable @escaping () async -> DiscardedResult
176+
operation: __owned @Sendable @escaping () async -> Void
177177
) {
178178
#if SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY
179179
let flags = taskCreateFlags(
180180
priority: priority, isChildTask: true, copyTaskLocals: false,
181181
inheritContext: false, enqueueJob: false,
182-
addPendingGroupTaskUnconditionally: true
182+
addPendingGroupTaskUnconditionally: true, isDiscardingTask: true
183183
)
184184
#else
185185
let flags = taskCreateFlags(
186186
priority: priority, isChildTask: true, copyTaskLocals: false,
187187
inheritContext: false, enqueueJob: true,
188-
addPendingGroupTaskUnconditionally: true
188+
addPendingGroupTaskUnconditionally: true, isDiscardingTask: true
189189
)
190190
#endif
191191

@@ -206,9 +206,9 @@ public struct DiscardingTaskGroup {
206206
#if SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY
207207
@available(*, unavailable, message: "Unavailable in task-to-thread concurrency model", renamed: "addTask(operation:)")
208208
#endif
209-
public mutating func addTaskUnlessCancelled<DiscardedResult>(
209+
public mutating func addTaskUnlessCancelled(
210210
priority: TaskPriority? = nil,
211-
operation: __owned @Sendable @escaping () async -> DiscardedResult
211+
operation: __owned @Sendable @escaping () async -> Void
212212
) -> Bool {
213213
let canAdd = _taskGroupAddPendingTask(group: _group, unconditionally: false)
214214

@@ -220,13 +220,13 @@ public struct DiscardingTaskGroup {
220220
let flags = taskCreateFlags(
221221
priority: priority, isChildTask: true, copyTaskLocals: false,
222222
inheritContext: false, enqueueJob: false,
223-
addPendingGroupTaskUnconditionally: false
223+
addPendingGroupTaskUnconditionally: false, isDiscardingTask: true
224224
)
225225
#else
226226
let flags = taskCreateFlags(
227227
priority: priority, isChildTask: true, copyTaskLocals: false,
228228
inheritContext: false, enqueueJob: true,
229-
addPendingGroupTaskUnconditionally: false
229+
addPendingGroupTaskUnconditionally: false, isDiscardingTask: true
230230
)
231231
#endif
232232

@@ -237,13 +237,13 @@ public struct DiscardingTaskGroup {
237237
}
238238

239239
@_alwaysEmitIntoClient
240-
public mutating func addTask<DiscardedResult>(
241-
operation: __owned @Sendable @escaping () async -> DiscardedResult
240+
public mutating func addTask(
241+
operation: __owned @Sendable @escaping () async -> Void
242242
) {
243243
let flags = taskCreateFlags(
244244
priority: nil, isChildTask: true, copyTaskLocals: false,
245245
inheritContext: false, enqueueJob: true,
246-
addPendingGroupTaskUnconditionally: true
246+
addPendingGroupTaskUnconditionally: true, isDiscardingTask: true
247247
)
248248

249249
// Create the task in this group.
@@ -260,8 +260,8 @@ public struct DiscardingTaskGroup {
260260
@available(*, unavailable, message: "Unavailable in task-to-thread concurrency model", renamed: "addTaskUnlessCancelled(operation:)")
261261
#endif
262262
@_alwaysEmitIntoClient
263-
public mutating func addTaskUnlessCancelled<DiscardedResult>(
264-
operation: __owned @Sendable @escaping () async -> DiscardedResult
263+
public mutating func addTaskUnlessCancelled(
264+
operation: __owned @Sendable @escaping () async -> Void
265265
) -> Bool {
266266
#if compiler(>=5.5) && $BuiltinCreateAsyncTaskInGroup
267267
let canAdd = _taskGroupAddPendingTask(group: _group, unconditionally: false)
@@ -274,7 +274,7 @@ public struct DiscardingTaskGroup {
274274
let flags = taskCreateFlags(
275275
priority: nil, isChildTask: true, copyTaskLocals: false,
276276
inheritContext: false, enqueueJob: true,
277-
addPendingGroupTaskUnconditionally: false
277+
addPendingGroupTaskUnconditionally: false, isDiscardingTask: true
278278
)
279279

280280
// Create the task in this group.
@@ -547,15 +547,15 @@ public struct ThrowingDiscardingTaskGroup<Failure: Error> {
547547
@available(*, unavailable, message: "Unavailable in task-to-thread concurrency model", renamed: "addTask(operation:)")
548548
#endif
549549
@_alwaysEmitIntoClient
550-
public mutating func addTask<DiscardedResult>(
550+
public mutating func addTask(
551551
priority: TaskPriority? = nil,
552-
operation: __owned @Sendable @escaping () async throws -> DiscardedResult
552+
operation: __owned @Sendable @escaping () async throws -> Void
553553
) {
554554
#if compiler(>=5.5) && $BuiltinCreateAsyncTaskInGroup
555555
let flags = taskCreateFlags(
556556
priority: priority, isChildTask: true, copyTaskLocals: false,
557557
inheritContext: false, enqueueJob: true,
558-
addPendingGroupTaskUnconditionally: true
558+
addPendingGroupTaskUnconditionally: true, isDiscardingTask: true
559559
)
560560

561561
// Create the task in this group.
@@ -569,9 +569,9 @@ public struct ThrowingDiscardingTaskGroup<Failure: Error> {
569569
@available(*, unavailable, message: "Unavailable in task-to-thread concurrency model", renamed: "addTask(operation:)")
570570
#endif
571571
@_alwaysEmitIntoClient
572-
public mutating func addTaskUnlessCancelled<DiscardedResult>(
572+
public mutating func addTaskUnlessCancelled(
573573
priority: TaskPriority? = nil,
574-
operation: __owned @Sendable @escaping () async throws -> DiscardedResult
574+
operation: __owned @Sendable @escaping () async throws -> Void
575575
) -> Bool {
576576
#if compiler(>=5.5) && $BuiltinCreateAsyncTaskInGroup
577577
let canAdd = _taskGroupAddPendingTask(group: _group, unconditionally: false)
@@ -584,7 +584,7 @@ public struct ThrowingDiscardingTaskGroup<Failure: Error> {
584584
let flags = taskCreateFlags(
585585
priority: priority, isChildTask: true, copyTaskLocals: false,
586586
inheritContext: false, enqueueJob: true,
587-
addPendingGroupTaskUnconditionally: false
587+
addPendingGroupTaskUnconditionally: false, isDiscardingTask: true
588588
)
589589

590590
// Create the task in this group.

‎stdlib/public/Concurrency/Task.cpp

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -940,6 +940,7 @@ static AsyncTaskAndContext swift_task_create_commonImpl(
940940
// be is the final hop. Store a signed null instead.
941941
initialContext->Parent = nullptr;
942942

943+
// FIXME: add discarding flag
943944
concurrency::trace::task_create(
944945
task, parent, group, asyncLet,
945946
static_cast<uint8_t>(task->Flags.getPriority()),
@@ -1060,21 +1061,40 @@ void swift::swift_task_run_inline(OpaqueValue *result, void *closureAFP,
10601061

10611062
SWIFT_CC(swift)
10621063
AsyncTaskAndContext swift::swift_task_create(
1063-
size_t taskCreateFlags,
1064+
size_t rawTaskCreateFlags,
10641065
TaskOptionRecord *options,
10651066
const Metadata *futureResultType,
10661067
void *closureEntry, HeapObject *closureContext) {
1067-
FutureAsyncSignature::FunctionType *taskEntry;
1068-
size_t initialContextSize;
1069-
std::tie(taskEntry, initialContextSize) =
1068+
TaskCreateFlags taskCreateFlags(rawTaskCreateFlags);
1069+
1070+
if (taskCreateFlags.isDiscardingTask()) {
1071+
ThinNullaryAsyncSignature::FunctionType *taskEntry;
1072+
size_t initialContextSize;
1073+
1074+
std::tie(taskEntry, initialContextSize) =
10701075
getAsyncClosureEntryPointAndContextSize<
1071-
FutureAsyncSignature,
1072-
SpecialPointerAuthDiscriminators::AsyncFutureFunction>(closureEntry);
1076+
ThinNullaryAsyncSignature,
1077+
SpecialPointerAuthDiscriminators::AsyncThinNullaryFunction>(closureEntry);
1078+
1079+
return swift_task_create_common(
1080+
rawTaskCreateFlags, options, futureResultType,
1081+
reinterpret_cast<TaskContinuationFunction *>(taskEntry), closureContext,
1082+
initialContextSize);
10731083

1074-
return swift_task_create_common(
1075-
taskCreateFlags, options, futureResultType,
1076-
reinterpret_cast<TaskContinuationFunction *>(taskEntry), closureContext,
1077-
initialContextSize);
1084+
} else {
1085+
FutureAsyncSignature::FunctionType *taskEntry;
1086+
size_t initialContextSize;
1087+
1088+
std::tie(taskEntry, initialContextSize) =
1089+
getAsyncClosureEntryPointAndContextSize<
1090+
FutureAsyncSignature,
1091+
SpecialPointerAuthDiscriminators::AsyncFutureFunction>(closureEntry);
1092+
1093+
return swift_task_create_common(
1094+
rawTaskCreateFlags, options, futureResultType,
1095+
reinterpret_cast<TaskContinuationFunction *>(taskEntry), closureContext,
1096+
initialContextSize);
1097+
}
10781098
}
10791099

10801100
#ifdef __ARM_ARCH_7K__

‎stdlib/public/Concurrency/Task.swift

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,38 @@ func taskCreateFlags(
525525
return bits
526526
}
527527

528+
/// Form task creation flags for use with the createAsyncTask builtins.
529+
@available(SwiftStdlib 5.9, *)
530+
@_alwaysEmitIntoClient
531+
func taskCreateFlags(
532+
priority: TaskPriority?, isChildTask: Bool, copyTaskLocals: Bool,
533+
inheritContext: Bool, enqueueJob: Bool,
534+
addPendingGroupTaskUnconditionally: Bool,
535+
isDiscardingTask: Bool
536+
) -> Int {
537+
var bits = 0
538+
bits |= (bits & ~0xFF) | Int(priority?.rawValue ?? 0)
539+
if isChildTask {
540+
bits |= 1 << 8
541+
}
542+
if copyTaskLocals {
543+
bits |= 1 << 10
544+
}
545+
if inheritContext {
546+
bits |= 1 << 11
547+
}
548+
if enqueueJob {
549+
bits |= 1 << 12
550+
}
551+
if addPendingGroupTaskUnconditionally {
552+
bits |= 1 << 13
553+
}
554+
if isDiscardingTask {
555+
bits |= 1 << 14
556+
}
557+
return bits
558+
}
559+
528560
// ==== Task Creation ----------------------------------------------------------
529561
@available(SwiftStdlib 5.1, *)
530562
extension Task where Failure == Never {

‎stdlib/public/Concurrency/TaskGroup.cpp

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464

6565
using namespace swift;
6666

67-
#if 0
67+
#if 1
6868
#define SWIFT_TASK_GROUP_DEBUG_LOG(group, fmt, ...) \
6969
fprintf(stderr, "[%#lx] [%s:%d][group(%p%s)] (%s) " fmt "\n", \
7070
(unsigned long)Thread::current().platformThreadId(), \
@@ -228,7 +228,7 @@ class TaskGroupBase : public TaskGroupTaskStatusRecord {
228228
reinterpret_cast<OpaqueValue *>(fragment->getError()) :
229229
fragment->getStoragePtr(),
230230
/*successType=*/fragment->getResultType(),
231-
/*task=*/asyncTask
231+
/*retainedTask==*/asyncTask
232232
};
233233
}
234234

@@ -1040,7 +1040,7 @@ static void fillGroupNextResult(TaskFutureWaitAsyncContext *context,
10401040

10411041
case PollStatus::Error: {
10421042
auto error = reinterpret_cast<SwiftError *>(result.storage);
1043-
fillGroupNextErrorResult(context, error);
1043+
fillGroupNextErrorResult(context, error); // FIXME: this specifically retains the error, but likely should not!??!!?
10441044
return;
10451045
}
10461046

@@ -1146,7 +1146,10 @@ void AccumulatingTaskGroup::offer(AsyncTask *completedTask, AsyncContext *contex
11461146
// will need to release in the other path.
11471147
lock(); // TODO: remove fragment lock, and use status for synchronization
11481148

1149-
SWIFT_TASK_GROUP_DEBUG_LOG(this, "offer, completedTask:%p , status:%s", completedTask, statusString().c_str());
1149+
SWIFT_TASK_GROUP_DEBUG_LOG(this, "offer, completedTask:%p (count:%d), status:%s",
1150+
completedTask,
1151+
swift_retainCount(completedTask),
1152+
statusString().c_str());
11501153

11511154
// Immediately increment ready count and acquire the status
11521155
//
@@ -1218,7 +1221,6 @@ void DiscardingTaskGroup::offer(AsyncTask *completedTask, AsyncContext *context)
12181221
// We can do this, since in this mode there is no ready count to keep track of,
12191222
// and we immediately discard the result.
12201223
auto afterComplete = statusCompletePendingAssumeRelease();
1221-
(void) afterComplete;
12221224
const bool alreadyDecrementedStatus = true;
12231225
SWIFT_TASK_GROUP_DEBUG_LOG(this, "offer, complete, status afterComplete:%s", afterComplete.to_string(this).c_str());
12241226

@@ -1291,6 +1293,14 @@ void DiscardingTaskGroup::offer(AsyncTask *completedTask, AsyncContext *context)
12911293
// This is the last task, we have a waiting task and there was no error stored previously;
12921294
// We must resume the waiting task with a success, so let us return here.
12931295
resumeWaitingTask(completedTask, assumed, /*hadErrorResult=*/false, alreadyDecrementedStatus);
1296+
1297+
// // TODO: since the DiscardingTaskGroup ended up written as `-> T` in order to use the same pointer auth as the
1298+
// // usual task closures; we end up retaining the value when it is returned. As this is a discarding group
1299+
// // we actually can and should release this value.
1300+
// // Is there a way we could avoid the retain made on the returned value entirely?
1301+
// if (completedTask->futureFragment()->getResultType()->isClassObject()) {
1302+
// swift_release(reinterpret_cast<HeapObject *>(completedTask->futureFragment()->getStoragePtr()));
1303+
// }
12941304
}
12951305
} else {
12961306
// it wasn't the last pending task, and there is no-one to resume;
@@ -1351,14 +1361,17 @@ void TaskGroupBase::resumeWaitingTask(
13511361
// Run the task.
13521362
auto result = PollResult::get(completedTask, hadErrorResult);
13531363
SWIFT_TASK_GROUP_DEBUG_LOG(this,
1354-
"resume waiting DONE, task = %p, backup = %p, error:%d, complete with = %p, status = %s",
1355-
waitingTask, backup, hadErrorResult, completedTask, statusString().c_str());
1364+
"resume waiting DONE, task = %p, error:%d, complete with = %p, status = %s",
1365+
waitingTask, hadErrorResult, completedTask, statusString().c_str());
13561366

13571367
auto waitingContext =
13581368
static_cast<TaskFutureWaitAsyncContext *>(
13591369
waitingTask->ResumeContext);
1370+
SWIFT_TASK_GROUP_DEBUG_LOG(this, "completed task %p", completedTask);
1371+
13601372

13611373
fillGroupNextResult(waitingContext, result);
1374+
SWIFT_TASK_GROUP_DEBUG_LOG(this, "completed task %p; AFTER FILL", completedTask);
13621375

13631376
// Remove the child from the task group's running tasks list.
13641377
// The parent task isn't currently running (we're about to wake
@@ -1368,7 +1381,18 @@ void TaskGroupBase::resumeWaitingTask(
13681381
// does a parent -> child traversal while recursively holding
13691382
// locks) because we know that the child task is completed and
13701383
// we can't be holding its locks ourselves.
1384+
auto before = completedTask;
13711385
_swift_taskGroup_detachChild(asAbstract(this), completedTask);
1386+
SWIFT_TASK_GROUP_DEBUG_LOG(this, "completedTask %p; AFTER DETACH (count:%d)", completedTask, swift_retainCount(completedTask));
1387+
if (hadErrorResult) {
1388+
SWIFT_TASK_GROUP_DEBUG_LOG(this, "BEFORE RELEASE error task=%p (count:%d)\n",
1389+
completedTask,
1390+
swift_retainCount(completedTask));
1391+
// We only used the task to keep the error in the future fragment around
1392+
// so now that we emitted the error and detached the task, we are free to release the task immediately.
1393+
auto error = reinterpret_cast<SwiftError *>(result.storage);
1394+
swift_release(completedTask); // we need to do this if the error is a class
1395+
}
13721396

13731397
_swift_tsan_acquire(static_cast<Job *>(waitingTask));
13741398
// TODO: allow the caller to suggest an executor
@@ -1377,7 +1401,7 @@ void TaskGroupBase::resumeWaitingTask(
13771401
#endif /* SWIFT_CONCURRENCY_TASK_TO_THREAD_MODEL */
13781402
} else {
13791403
SWIFT_TASK_GROUP_DEBUG_LOG(this, "CAS failed, task = %p, backup = %p, complete with = %p, status = %s",
1380-
waitingTask, backup, completedTask, statusString().c_str());
1404+
waitingTask, completedTask, statusString().c_str());
13811405
}
13821406
}
13831407
}

‎stdlib/public/runtime/HeapObject.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,13 +526,15 @@ void (*SWIFT_RT_DECLARE_ENTRY _swift_release)(HeapObject *object) =
526526
_swift_release_;
527527

528528
void swift::swift_nonatomic_release(HeapObject *object) {
529+
fprintf(stderr, "[%s:%d](%s) swift_nonatomic_release %p\n", __FILE_NAME__, __LINE__, __FUNCTION__, object);
529530
SWIFT_RT_TRACK_INVOCATION(object, swift_nonatomic_release);
530531
if (isValidPointerForNativeRetain(object))
531532
object->refCounts.decrementAndMaybeDeinitNonAtomic(1);
532533
}
533534

534535
SWIFT_ALWAYS_INLINE
535536
static void _swift_release_n_(HeapObject *object, uint32_t n) {
537+
fprintf(stderr, "[%s:%d](%s) _swift_release_n_ %p (n=%d)\n", __FILE_NAME__, __LINE__, __FUNCTION__, object, n);
536538
SWIFT_RT_TRACK_INVOCATION(object, swift_release_n);
537539
if (isValidPointerForNativeRetain(object))
538540
object->refCounts.decrementAndMaybeDeinit(n);
@@ -796,6 +798,8 @@ void swift::swift_unownedCheck(HeapObject *object) {
796798
}
797799

798800
void _swift_release_dealloc(HeapObject *object) {
801+
fprintf(stderr, "[%s:%d](%s) _swift_release_dealloc %p (count before: %d)\n", __FILE_NAME__, __LINE__, __FUNCTION__, object,
802+
swift_retainCount(object));
799803
asFullMetadata(object->metadata)->destroy(object);
800804
}
801805

‎stdlib/toolchain/Compatibility56/include/Runtime/Concurrency.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ struct AsyncTaskAndContext {
5151
AsyncContext *InitialContext;
5252
};
5353

54-
5554
/// Caution: not all future-initializing functions actually throw, so
5655
/// this signature may be incorrect.
5756
using FutureAsyncSignature =

0 commit comments

Comments
 (0)