[Concurrency] Defensive copying of task locals in task groups, rather than crashing

[ktoso]

This is a more advanced version of #73881

This addresses a memory safety issue that task groups and task locals have faced since their introduction and has been previously guarded against by crashing rather than allowing the unsafety. The very specific situation this is about is the following:

      await withTaskGroup(of: Void.self) { group in
        TL.$two.withValue(2222) { // PREVIOUSLY: crash and warn against this usage pattern
          group.addTask {
            print("Survived, two: \(TL.two)")
          }
        }

The problem is that the withValue is effectively a push and a pop of the task local binding. The and child tasks refer directly to their parent's task local storage in order to minimize copying. This is fantastic because operations such as this:

TL.$a.withValue(...) {
TL.$b.withValue(...) {
TL.$c.withValue(...) {
TL.$d.withValue(...) { 
await withTaskGroup(of: Void.self) { group in
  group.addTask {}  // no copying, direct to parent reference (safe and good)
}

will cause zero copying. (good and safe)

However, the case of DIRECTLY wrapping an addTask with a task local binding like this: TL.$two.withValue(2222) { group.addTask {} } is unsafe because the "pop" of the $two binding would happen BEFORE the child task gets to run to completion -- would would allow it to refer to released memory.

Previous solution:
We previously would crash whenever withValue was used inside a task group at all. Which led to problematic and unexpected crashes when more complex libraries which wrap code in task groups would call to user code or set task locals, causing people to work around issues: swiftlang/swift-testing#339 and a few more in server libraries.

Sometimes they may not be able to work around these incompatibilities though, leading to a not well composing structured task hierarchy (bad!)

This solution:
We instead now detect task locals pushed during the body of a task group. Then, as we addTask we defensively COPY them if directly in the parent task. This way the following situation is safe:

  await TL.$one.withValue(11) {
    await TL.$one.withValue(1111) {
      await withTaskGroup(of: Void.self) { group in

        TL.$two.withValue(2222) {
          group.addTask { 
          // task locals:
          // - copy 2222 item
          // - next reference to 1111
          // [skip] 11 no need to refer to it, since one already bound   
          }
        }

etc.

TL;DR:

• Instead of crashing in order to prevent bad memory access, we not handle it by copying.
• The situation where we copy is very rare and does not impact most of users: because the usual way is to bind around the task group
• It is simple to avoid the copying: move from withValue { addTask {} } to addTask { withValue {} } which causes no copying.

Result: Instead of crashing, we now can support this pattern - which sometimes comes up unbeknownst to actual developers, because they may be using libraries which wrap them in task groups and whatnot.

resolves rdar://127156303

[ktoso]

@swift-ci please test

[ktoso]

@swift-ci please test

[ktoso]

@swift-ci please test

[ktoso]

was producing warnings, i had introduced this recently -- sorry for noise

[ktoso]

@swift-ci please test

[ktoso]

@swift-ci please smoke test

[ktoso]

resolves #73217

[ktoso]

@swift-ci please smoke test macOS

[ktoso]

@swift-ci please smoke test macOS

[ktoso]

@swift-ci please smoke test macOS

[rjmccall]

So, this is eagerly copying everything that was pushed inside of any task group, right? That's actually subtly less than optimal in the case where there are multiple task groups active at once — we only really need to copy up to the task group that the task is being created within. It's fine to do that for now, but let's not do anything that commits us to that in the long term, like making anything about this public ABI. (This doesn't need to be public anyway — it's all internal to the concurrency library.)

[rjmccall]

You don't need to do this now, but it kind of sounds like these are flags instead of a single "kind".

[ktoso]

Hm, lemme think if I can put these somewhere else 🤔

[rjmccall]

A || B && C is parsed as A || (B && C). That turns out to be logically equivalent in this case, but still, it's better to get into the habit of parenthesizing assertion conditions.

The assertion and behavior here is strange — the method name suggests that it's just changing the next pointer, but you're really assuming a stronger condition. It feels like you can't really assert that condition properly, which to me is usually a sign that the operation isn't as general as I'm thinking it is. That's okay; operations don't always generalize. It just suggests that you ought to name this method something that indicates its special behavior, and maybe feel less bad about writing something that doesn't feel like a generic operation.

I assume what we're doing here is cloning the entries and then calling relinkNext on them.

[ktoso]

I think it's a good point to rename this method, it is a very specific trick. When we detect the "next" task local item is "safe" as in "not the bad pattern where we have to copy" we immediately link to it rather than continuing our copying dance. I'll rename and see if I can clarify some more.

Fixing the assertion too, thank you for noticing that

[ktoso]

cleaned up in #74089

[rjmccall]

This is a logically-unrelated cleanup? Seems reasonable, just trying to find the connection.

[ktoso]

Yes, this bit is just cleanup -- same functionality.

[rjmccall]

I don't think this needs to be an exported runtime function.

[ktoso]

I think we can managed without exposing it -- will remove (and remove before getting this into 6.0)

[ktoso]

removed in #74089

[ktoso]

Thanks for the review John!

So, this is eagerly copying everything that was pushed inside of any task group, right? That's actually subtly less than optimal in the case where there are multiple task groups active at once
Right, we can optimize this a bit more eventually, I didn't find a good way to do this yet, we'd have to store "which group it was created in" and I guess we'd probably either need to make task local 'item' larger or find a different trick, like another kind of record...

I'll make sure we don't lock ourselfs out of the optimization, following up now

[ktoso]

renamed this in follow up #74089