Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The constructs provided by this package need to be marked Sendable #45

Closed
2 tasks done
lorentey opened this issue Sep 23, 2021 · 6 comments · Fixed by #46
Closed
2 tasks done

The constructs provided by this package need to be marked Sendable #45

lorentey opened this issue Sep 23, 2021 · 6 comments · Fixed by #46
Labels
bug Something isn't working
Milestone

Comments

@lorentey
Copy link
Member

Swift 5.5 introduces Sendable, which puts new constraints on transferring values across concurrency domains.

The constructs provided by this package are designed to be safe to use without synchronization, so they should be marked Sendable. (Hopefully people aren't passing around naked ManagedAtomic values, but in case they do, the compiler should know that this is safe.)

Information

  • Package version: 1.0.1
  • Platform version: iOS 15 & macOS Monterey
  • Swift version: 5.5

Checklist

  • If possible, I've reproduced the issue using the main branch of this package.
  • I've searched for existing reports of the same issue.
@lorentey lorentey added the bug Something isn't working label Sep 23, 2021
@lorentey lorentey modified the milestone: 1.0.2 Sep 23, 2021
@lorentey
Copy link
Member Author

The big question that needs to be answered is this: do we need to restrict ManagedAtomic<T>/UnsafeAtomic<T> to be only marked Sendable if T itself is also Sendable?

For example, SomeClass below is very much not safe to transfer across concurrency domains, so it looks like an atomic reference to it shouldn't be, either.

class SomeClass: AtomicReference {
  var array: [Int] = []
}

let v: ManagedAtomic<SomeClass> = ...
let instance = v.load(ordering: .sequentiallyConsistent) // OK
instance.array.append(42) // data race

@lorentey
Copy link
Member Author

Putting this back on 1.0.2 -- we'll need to decide whether this can technically go in a patch release, but I think we'll want to release this together with #41 in any case. We may just need to rename the milestone to 1.1.0.

@lorentey
Copy link
Member Author

This is more urgent than I expected, as UnsafeAtomic has gained an implicit, unconditional, Sendable conformance in the Swift 5.5 release. The implicit conformance isn't right, and the longer we wait to fix it, the more likely it becomes that someone starts depending on it.

@lorentey lorentey modified the milestones: 1.0.2, 1.1.0 Sep 24, 2021
@lorentey
Copy link
Member Author

Filed #47 to track the UnsafeAtomic issue, which we will need to fix as soon as possible.

Fixing the Sendable conformance for UnsafeAtomic can be done in an emergency patch release, but conforming ManagedAtomic to Sendable can only be done in a new minor release.

@vincentisambart
Copy link

The new Swift Concurrency Checking warnings in Xcode 14 (in Targeted mode) told me that my ManagedAtomicLazyReference are not sendable, even though it's one of the main reason to use this package, so having this fixed would be great 😁

@stevapple
Copy link

When trying to adopt Atomics in Swift NIO, we see CI failure because of missing Sendable conformance in Swift 5.5, where @preconcurrency is not supported.

Since we cannot drop 5.5 at the time, it’s better to add the conformance in Atomics, which holds all the managed atomic types. These values are not intended for production, but instead as part of the test utilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants