Skip to content

Conversation

sebsto
Copy link
Contributor

@sebsto sebsto commented Feb 16, 2025

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

@sebsto sebsto added the semver/none No version bump required. label Feb 16, 2025
@sebsto sebsto self-assigned this Feb 16, 2025
// if the iterator is waiting for an element, give it to it
// otherwise, enqueue the element
if let continuation = _continuation {
continuation.resume(returning: invocation)
Copy link
Contributor

@t089 t089 Feb 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +381 to +391
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sebsto changed the title Fix possible data race in LocalServer's invocation pool Fix data race in LocalServer's invocation pool Feb 26, 2025
@sebsto
Copy link
Contributor Author

sebsto commented Feb 27, 2025

Thank you @t089 for your help.
After discussion with @fabianfett, I prefer to close this one in favour of #486

@sebsto sebsto closed this Feb 27, 2025
@sebsto sebsto deleted the sebsto/pool branch March 1, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants