AsyncQueue fix and improvements - #52
Conversation
- Wrap PendingTask removal in `defer` so a thrown or cancelled operation no longer leaves stale entries in `tasksByMetadata`. - The "depend only on the bucket's last task" optimization checked `metadata.isDependency(of: metadata)` (the new task's self-dep) instead of `pendingMetadata.isDependency(of: pendingMetadata)` (the bucket's). With cross-metadata dependencies, the new task could start before earlier concurrent tasks in that bucket.
Inline the PendingTasks helper so the dictionary lives directly inside a Mutex<...> property of AsyncQueue. Drops the NSLock + nonisolated(unsafe) pair, removes the helper class, and lets the compiler verify Sendability without the misleading "Unchecked sendable" comment. The "AsyncQueue" lock name is lost since Mutex doesn't carry one. The inner Task closure now captures self implicitly (AsyncQueue is Sendable) instead of the helper instance; memory ownership is equivalent.
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Nice findings 🤩
Just a few comments on the tests.
| /// - `.serial` is self-serializing | ||
| /// - `.concurrent` is a dependency of `.serial` (but not vice versa) | ||
| /// | ||
| /// This is the configuration that exposes the bucket-self-dependency bug: |
There was a problem hiding this comment.
I don’t think we should reference the … bug. It’s fixed by this PR.
| } | ||
| } | ||
|
|
||
| /// Regression test: a `.serial` task depending on a non-self-serializing |
There was a problem hiding this comment.
Regression test also doesn’t make sense as a comment going forward
| serialRan.value = true | ||
| } | ||
|
|
||
| // Release the last concurrent task. With the previous (buggy) |
There was a problem hiding this comment.
Again, please don’t reference a previous implementation.
| // Release the last concurrent task. With the previous (buggy) | ||
| // optimization, the serial task waited only on the bucket's last entry, | ||
| // so this would let it proceed. | ||
| cont3.yield() |
There was a problem hiding this comment.
Do we even need to yield values here? Shouldn’t finishing the continuation be sufficient?
There was a problem hiding this comment.
You are absolutely right. finish() is enough.
| // For self-serializing metadata, only the latest task matters as a dependency — | ||
| // it transitively covers all previous ones. Replace rather than append. | ||
| if metadata.isDependency(of: metadata) { | ||
| tasksByMetadata[metadata] = [PendingTask(task: task, id: id)] |
There was a problem hiding this comment.
This changes the semantics of tasksByMetadata because it now no longer has a reference to all the pending tasks for that metadata. I don’t think that’s an issue but the doc comment would need to be updated and also tasksByMetadata probably isn’t the best name for the variable anymore.
There was a problem hiding this comment.
Ah sorry I mixed a WIP changes. Reverting it..
Verifies the bucket-self-dependency optimization waits on every entry in the bucket, not just the last.
e1fca19 to
151e6fc
Compare
2 participants
PendingTaskentries were removed fromtasksByMetadataonly afteroperation()returned successfully. If the operation threw or the task was cancelled, the entry stayed forever, growing the dictionary unboundedly per failure. Fixed by moving the cleanup into adefer.metadata.isDependency(of: metadata)(the new task's self-dependency) instead ofpendingMetadata.isDependency(of: pendingMetadata)(the bucket's). With cross-metadata dependencies where the new kind serializes with itself but the bucket's kind doesn't, the new task could start before earlier concurrent tasks in that bucket finished.NSLocktoSynchronization.Mutex. ThePendingTaskshelper class is gone; the dictionary now lives directly inside aMutex<...>property ofAsyncQueue. Drops theNSLock+nonisolated(unsafe) varpair and lets the compiler verifySendableproperly.