Fix race in DemandBuffer #3475
Merged
Fix race in DemandBuffer #3475
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kabiroberai
commented
Nov 1, 2024
|
|
||
| final class DemandBuffer<S: Subscriber>: @unchecked Sendable { | ||
| private var buffer = [S.Input]() | ||
| private let subscriber: S | ||
| private var completion: Subscribers.Completion<S.Failure>? | ||
| private var demandState = Demand() | ||
| private let lock: os_unfair_lock_t | ||
| private let lock = NSRecursiveLock() |
There was a problem hiding this comment.
Once again, I wanted to err on the side of caution and changed this to a recursive lock to avoid deadlocks — but it might be worth reasoning about the code more thoroughly and determining if it's possible to change this back. The main concern is that there are calls to Subscriber.receive while the lock is held, and afaik these can invoke arbitrary code that might call back into the DemandBuffer and deadlock us.
jpsim
approved these changes
Nov 2, 2024
|
Indeed seems like an important fix to add. Great job @kabiroberai on catching this and #3447 ! |
|
CI failure seems unrelated to the PR 🤔 > sudo xcode-select -s /Applications/Xcode_16.0.app
xcode-select: error: invalid developer directory '/Applications/Xcode_16.0.app' |
|
GitHub may have changed their images, Xcode 16.0 is now named Xcode_16.app, with a symlink to Xcode_16.0.app, which I guess doesn't work for setting the directory. |
ChloeMayhewELT
pushed a commit
to ChloeMayhewELT/swift-composable-architecture
that referenced
this pull request
Jan 8, 2025
See also: CombineCommunity/CombineExt#90 Co-authored-by: Stephen Celis <stephen@stephencelis.com> # Conflicts: # Sources/ComposableArchitecture/Internal/Create.swift
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After landing #3447, we found another race condition — this time in
DemandBuffer, which is marked@unchecked Sendable(and should indeed be thread safe) but has no synchronization aroundDemandBuffer.buffer(as well as other slightly less critical properties) in some spots.A similar fix was actually applied to the original
CombineExt.DemandBufferimpl back in 2021, but wasn't ported over to TCA. See CombineCommunity/CombineExt#90.