From d1ac78cee5feb2df5bd7a695f9ceb59f5c150e97 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Mon, 8 Jul 2024 23:09:13 +0900 Subject: [PATCH 1/2] [Concurrency] TaskExecutors may be non-swift objects; dont swift_release them Since we introduced proper ownership of task executors, they are now released and retained. The problem appears with a dispatch_queue_t which conforms to TaskExecutor being passed to Task initializer and retains work okey because it is __owned. However, upon destroy we swift_released the executor reference, which is incorrect as we must be using object specific release methods -- in this case the safe way to support all kinds of objects is `swift_unknownObjectRelease` resolves rdar://131151645 --- stdlib/public/Concurrency/TaskStatus.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/stdlib/public/Concurrency/TaskStatus.cpp b/stdlib/public/Concurrency/TaskStatus.cpp index d195ac66c807d..f2a31fc2b1087 100644 --- a/stdlib/public/Concurrency/TaskStatus.cpp +++ b/stdlib/public/Concurrency/TaskStatus.cpp @@ -772,7 +772,11 @@ void AsyncTask::dropInitialTaskExecutorPreferenceRecord() { // // This should not be done for withTaskExecutorPreference executors, // however in that case, we would not enter this function here to clean up. - swift_release(executorIdentityToRelease); + // + // NOTE: This MUST NOT assume that the object is a swift object (and use + // swift_release), because a dispatch_queue_t conforms to TaskExecutor, + // and may be passed in here; in which case swift_releasing it would be incorrect. + swift_unknownObjectRelease(executorIdentityToRelease); } /**************************************************************************/ From e83835b3919d51d8578b235a51b4176600cd8453 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 9 Jul 2024 16:07:19 +0900 Subject: [PATCH 2/2] [Concurrency] Add test for objc type TaskExecutor --- .../async_task_executor_nsobject.swift | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 test/Concurrency/Runtime/async_task_executor_nsobject.swift diff --git a/test/Concurrency/Runtime/async_task_executor_nsobject.swift b/test/Concurrency/Runtime/async_task_executor_nsobject.swift new file mode 100644 index 0000000000000..ae8f31b3bb7dd --- /dev/null +++ b/test/Concurrency/Runtime/async_task_executor_nsobject.swift @@ -0,0 +1,49 @@ +// RUN: %target-run-simple-swift( -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library ) + +// REQUIRES: executable_test +// REQUIRES: concurrency +// REQUIRES: libdispatch + +// REQUIRES: OS=macosx +// REQUIRES: objc_interop + +// REQUIRES: concurrency_runtime +// UNSUPPORTED: back_deployment_runtime + +import Dispatch +import StdlibUnittest +import _Concurrency + +import Foundation +import Darwin + +// Sneaky trick to make this type objc reference counted. +// +// This test specifically checks that our reference counting accounts for existence of +// objective-c types as TaskExecutors -- which was a bug where we'd swift_release +// obj-c excecutors by accident (rdar://131151645). +final class NSQueueTaskExecutor: NSData, TaskExecutor, @unchecked Sendable { + public func enqueue(_ _job: consuming ExecutorJob) { + let job = UnownedJob(_job) + DispatchQueue.main.async { + job.runSynchronously(on: self.asUnownedTaskExecutor()) + } + } +} + +@main struct Main { + static func main() async { + var taskExecutor: (any TaskExecutor)? = NSQueueTaskExecutor() + + let task = Task(executorPreference: taskExecutor) { + dispatchPrecondition(condition: .onQueue(DispatchQueue.main)) + try? await Task.sleep(for: .seconds(2)) + return 12 + } + + taskExecutor = nil + + let num = await task.value + assert(num == 12) + } +}