Add Sendable conformance to association types

[seanmrich]

In the associations section of the documentation there is this code on how to define associations:

extension Author {
  static let books = hasMany(Book.self)
//           ^ Warning: Static property 'books' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor; this is an error in Swift 6
  ...
}

When using strict concurrency since the implementation of SE-0412 you get a warning as annotated in the code above. A workaround is to write the declarations as static var rather than static let, but then you have to fully specify the type and thereby lose the nice ergonomics of the example above. Would it be possible to conform types like BelongsToAssociation and HasManyAssociation to Sendable?

[groue]

Thanks for the report, @seanmrich. I haven't installed the latest beta yet.

Strictly speaking, associations are not Sendable, because they can embed non-Sendable values or closures. See for example the family of methods name -WhenConnected in DerivableRequest: anything can happen, there. TableAlias is not Sendable either, and requires a rework.

Practically speaking, I'm sure that most apps define requests and associations that only embed values and closures that are free of data races.

Still, making them strictly Sendable would introduce a lot of breaking changes, that will require bumping GRDB to GRDB 7.

I'm torn. I wish we could find a step-by-step way forward.

Besides changing let to var, did you try other workarounds? nonisolated(unsafe) (not sexy)? @preconcurrency import GRDB? Something else?

[seanmrich]

@preconcurrency doesn't fix the warning. I don't think I fully understand how this works and it has rarely done anything for libraries I've tried it with. Perhaps because I'm using strict concurrency, or maybe because there is already adoption of structured concurrency features in GRDB? Not sure.

nonisolated(unsafe) does remove the warning, but I don't feel comfortable forcing the override. Besides just turning off sendability checking, I'm not sure what else would affect it. My impression is that constructing these associations on demand shouldn't be a significant burden unless I'm missing something?

Looking through the code supporting associations, it looks like many of the model types are trivially Sendable now. Perhaps that's a starting point? As you mentioned, TableAlias looks like a sticking point and would need to be reconsidered (actor?). As Apple continues to turn the screws on data race safety, Sendable is going to spread virally through our code, especially something like GRDB.

Relatedly, the compiler is also warning about static declarations in GRDB/Core/Database.swift within ColumnType. This seems like one of those trivially Sendable types that could be converted to remove those warnings.

[groue]

Thank you for your checks.

Yes, the way forward is clearly to make Sendable as many GRDB types as possible. I have already explored what it takes, and this is how I know that this can't be done without introducing breaking changes.

Errr... 🤔 Actually I'm not 100% sure if the required changes would introduce hard errors or warnings. Warnings are not a breaking change, but hard errors are (a user upgrades GRDB and their app stops compiling - that's what we want to avoid).

I just asked on the Swift forums.

[mattmassicotte]

Hello!

I am not a GRDB user, and I know basically zero about how it works. However, I am a Swift concurrency "enthusiast" and @groue got me interested because it turns out I do not understand what's going on.

First, why doesn't @preconcurrency fix the issue? This particular problem isn't related to the public interfaces of GRDB. The hasMany function returns a non-isolated and non-Sendable type. And that cannot be stored safely a static variable. Forget about Swift concurrency - what happens if I do this?

for i in 0..<1000 {
	DispatchQueue.global().async {
		Author.books.someActionOnBooks()
	}
}

Is this safe? If so, perhaps the type that hasMany returns actually is Sendable! And if not, could it be this is a latent (if unlikely) bug that the compiler is now forcing you to deal with?

[groue]

@seanmrich, you're correct to feel nervous about nonisolated(unsafe). But I will also try to soothe your concerns.

First, I really want to ship a GRDB version where associations are Sendable, and your original code compiles without any warning:

// GRDB 7
extension Author {
  // 😇 No warning because HasManyAssociation is Sendable
  static let books = hasMany(Book.self)
}

Meanwhile, a way to remove the warning is nonisolated(unsafe):

// GRDB 6
extension Author {
  // 😰
  nonisolated(unsafe) static let books = hasMany(Book.self)
}

Now let's soothe your concerns:

The hasMany(Book.self) association is completely safe. You will not have any problem at runtime. The type of this value is not marked as Sendable, which explains why the compiler warns. But this particular instance of association has all the runtime properties of Sendable values. It won't blow up your program, 100% guaranteed. But you have to trust my words, because the compiler warns.

And the compiler warns for a good reason: the current public apis of GRDB makes it possible to build non-sendable and unsafe associations. On the other hand, your app much probably does not.

For example:

extension Author {
  // 👎 A DANGEROUS ASSOCIATION
  static func wackyBooks(_ value: NonSendableType) -> HasManyAssociation<Author, Book> {
    Author.hasMany(Book.self).filter(Column("whatever") == value)
  }
}

let value: NonSendableType = ...
let authors = try await dbQueue.read { db in
  try Author
    .having(Author.wackyBooks(value).isEmpty) // 😅
    .fetchAll(db)
}

See? It is possible to define associations that involve non-sendable values, and write code that has this non-sendable value cross isolation domains in an unsafe way.

This possibility is the reason why GRDB associations are not Sendable today, and the reason why you need to use nonisolated(unsafe) in order to mute the current warning.

Fortunately, most requests and associations are built from plain strings, numbers, etc. You can audit your code for requests and associations built out of non-sendable values, but chances are low. And I'd bet that no association uses such values.

I hope I could convince you that decorating plain associatons with nonisolated(unsafe) is ugly, but does not create any risk.

In GRDB 7, the library will require that requests and associations are built from Sendable values. The compiler will warn on the definition of wackyBooks, above. But it will no longer warn on static let books = hasMany(Book.self).

---

EDIT: the wackyBooks association actually is safe, because it does not embed the non-sendable value (it embeds a database representation of the value, which is safe). The problem with the above sample code is not the association, but the fact that read() async throws does not require a @Sendable closure (it will in GRDB 7).

A really unsafe association would embed the non-sendable value inside it, for example with a closure:

extension Author {
  // 👿 DEMONIAC ASSOCIATION
  static func wackyBooks(_ value: NonSendableType) -> HasManyAssociation<Author, Book> {
    Author.hasMany(Book.self).filterWhenConnected { db in
      Column("whatever") == value
    }
  }
}

Now this one is nasty, especially if you use such an association with tools like database observation that will run the request at about any time from various database threads - typically a context where we really need Sendable :-)

Fortunately, nobody writes code like that.

[mattmassicotte]

Once sendability is supported, there’s no indication by the compiler that I should remove the annotation. Diligent reading of release notes is the only trigger. Am I wrong?

You are not wrong. But, the compiler team seems interested in catching this. However, I do want to point out that this is purely a cosmetic problem. If this is used to express safety that is just invisible to the compiler, like @groue explained in fantastic detail, nothing changes.

[groue]

Developing the skills to deal with these problems and getting comfortable with constructs like nonisolated(unsafe) should be a high priority for all Swift developers right now.

You're right, @mattmassicotte. I also sometimes feel like we're living in a bubble, surrounded by the most demanding developers who won't rest until they have a clear understanding of Swift concurrency and related techniques. This explains all our questions, frustrations, incessant explorations, speeches, writings, conversations with our peers... I sometimes wonder if this level of demand, impatience and rigor is normal and should be expected from all devs. 😅

[seanmrich]

This was all tremendously helpful for me. Concurrency is notoriously challenging. I’ve definitely screwed it up in the past so I tend to wring my hands a bit, especially since the idea of the compiler preventing data races (trying at least) is new to me. I truly appreciate the guidance.

[mattmassicotte]

I sometimes wonder if this level of demand, impatience and rigor is normal and should be expected from all devs.

I certainly hope not! But, changes like this don't happen very often.

The warnings are opt-in for a reason. There are huge trade-offs, but one advantage is avoiding confusion/frustration until Swift and Apple's own APIs are ready and they have some documentation on how everything is supposed to work. We're beta testers!