Fix data race in LocalServer's invocation pool #479
Conversation
| // if the iterator is waiting for an element, give it to it | ||
| // otherwise, enqueue the element | ||
| if let continuation = _continuation { | ||
| continuation.resume(returning: invocation) |
There was a problem hiding this comment.
Not sure if it is a good idea to resume a continuation while holding a lock... usually I would try to keep the withLock body free of side-effects.
| if let element = self.popFirst() { | ||
| // if there is an element in the buffer, dequeue it | ||
| return element | ||
| } else { | ||
| // we can't return nil if there is nothing to dequeue otherwise the async for loop will stop | ||
| // wait for an element to be enqueued | ||
| return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<T, any Error>) in | ||
| // store the continuation for later, when an element is enqueued | ||
| self._continuation.withLock { | ||
| $0 = continuation | ||
| // so, wait for an element to be enqueued | ||
| return try await withCheckedThrowingContinuation { | ||
| (continuation: CheckedContinuation<T, any Error>) in | ||
| self.mutex.withLock { mutexContent in | ||
| // store the continuation for later, when an element is enqueued | ||
| mutexContent.1 = continuation |
There was a problem hiding this comment.
This is still racy: between "popFirst" returning nil and storing the continuation, somebody could have already put sth in the buffer. Really, to implement this correctly, I think you need to acquire the lock, check your invariants, apply the necessary state change, release the lock and then run side-effects.
There was a problem hiding this comment.
The invariant here is: There can only be a stored continuation if the buffer is empty. If the buffer is non-empty, there cannot be a stored continuation.
sebsto
changed the title
|
Thank you @t089 for your help. |
Use a single Mutex to store both the Invocation buffer and the continuation to avoid a possible data race condition as described here.
https://forums.swift.org/t/how-to-use-withcheckedcontinuation-with-an-async-closure/77852/8