[Concurrency] Fix too optimistic TaskGroup bail-out-when-empty, #63016

ktoso · 2023-01-13T13:58:32Z

discarding group may need to emit an error out of such waitAll attempt, if a previous error was already stored.

The exact dance is:

we store an error from child task
the pending task count is 0
the code trying to waitAll would check status ONLY and bail out, noticing that pending count is zero -- returning the actual body value

Instead, it should:

check again the readyQueue if we're in, if there was a stored value, because if so, it may be an RawError that the DiscardingTaskGroup should emit.

This also is now able to easily handle the "first error always wins" but we choose to implement the "error thrown by body wins" after discussions within the team.

Resolves rdar://104198700
Resolves #63147
Resolves rdar://104507347
Resolves rdar://104332560
Resolves rdar://104212282

ktoso · 2023-01-13T13:59:39Z

This resolves a flaky test (and an actual bug) that @shahmishal and @beccadax reported today.
Thank you for the ping, was a real omission in the code.

ktoso · 2023-01-13T13:59:48Z

@swift-ci please smoke test

ktoso · 2023-01-13T14:02:57Z

I'll have another look and add more tests about the retain/release correctness of the errors... I have a suspicion it might be wrong and over-retain the error a bit... 🤔

ktoso · 2023-01-16T05:01:05Z

Had another bug hiding, we must offerError WHILE holding the group locks ofc, otherwise we get interleaving between the offer and completions of other tasks and may end up returning a success while we intended to throw the body error.

Fix: 95628b1

ktoso · 2023-01-16T09:38:25Z

@swift-ci please smoke test

ktoso · 2023-01-16T14:19:34Z

@swift-ci please smoke test

ktoso · 2023-01-17T02:01:36Z

@swift-ci please smoke test linux

ktoso · 2023-01-17T04:51:29Z

@swift-ci please smoke test

ktoso · 2023-01-17T06:17:45Z

@swift-ci please smoke test

ktoso · 2023-01-17T09:05:47Z

stdlib/public/Concurrency/TaskGroup.cpp

@@ -1641,8 +1652,9 @@ static void swift_taskGroup_waitAllImpl(
    ThrowingTaskFutureWaitContinuationFunction *resumeFunction,
    AsyncContext *rawContext) {
  auto waitingTask = swift_task_getCurrent();
-  waitingTask->ResumeTask = task_group_wait_resume_adapter;
-  waitingTask->ResumeContext = rawContext;


this was incorrect, we cannot do this without holding the lock, as otherwise an offer may pull the waiting task off the queue; but only one of the paths shall update and run the task.

ktoso · 2023-01-17T09:06:53Z

stdlib/public/Concurrency/TaskGroup.cpp

@@ -1739,21 +1739,52 @@ PollResult TaskGroupBase::tryEnqueueWaitingTask(AsyncTask *waitingTask) {
  bool haveRunOneChildTaskInline = false;

  reevaluate_if_TaskGroup_has_results:;
-  auto assumed = statusMarkWaitingAssumeAcquire();


The change here is that we don't mark and undo, but only mark when we indeed decide that we'll be waiting.

This is all still done with a lock, but the dance is more correct anyway.

ktoso · 2023-01-17T09:07:33Z

test/Concurrency/Runtime/async_taskgroup_discarding.swift

+
+@main struct Main {
+  static func main() async {
+    for i in 0...1_000 {


this is quick enough tbh, let's leave it like this

ktoso · 2023-01-18T13:10:36Z

@swift-ci please smoke test

ktoso · 2023-01-19T07:52:10Z

All passed but I want to make double-sure here, run again.

ktoso · 2023-01-19T07:52:15Z

@swift-ci please smoke test

may need to emit an error out of such waitAll attempt, if a previous error was already stored.

ktoso · 2023-01-24T00:39:48Z

@swift-ci please smoke test

ktoso · 2023-01-24T02:34:37Z

@swift-ci please smoke test linux

ktoso · 2023-01-24T04:17:58Z

Heh, only remaining thing is a pure virtual method called happening only on linux that I can't explain :-( Getting a linux box to dig deeper.

Only on our Ubuntu 20.04 CI this happens:

[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:847][group(0x55e4c18fc5b0)] (swift_taskGroup_initializeWithFlagsImpl) create group; flags: isDiscardingResults=1
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:631][group(0x55e4c18fc5b0,discardResults)] (statusAddPendingTaskRelaxed) addPending, after = TaskGroupStatus{ C:n W:n P:1 0000000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:631][group(0x55e4c18fc5b0,discardResults)] (statusAddPendingTaskRelaxed) addPending, after = TaskGroupStatus{ C:n W:n P:2 0000000000000000000000000000000000000000000000000000000000000010 }
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1159][group(0x55e4c18fc5b0,discardResults)] (offer) offer, completedTask:0x7fce8802a2f0, error:0, status:TaskGroupStatus{ C:n W:n P:2 0000000000000000000000000000000000000000000000000000000000000010 }
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1167][group(0x55e4c18fc5b0,discardResults)] (offer) offer, complete, status afterComplete:TaskGroupStatus{ C:n W:n P:1 0000000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1728][group(0x55e4c18fc5b0,discardResults)] (waitAll) waitAll, bodyError = (nil), status = TaskGroupStatus{ C:n W:n P:1 0000000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1743][group(0x55e4c18fc5b0,discardResults)] (waitAll) waitAll, status = TaskGroupStatus{ C:n W:n P:1 0000000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1792][group(0x55e4c18fc5b0,discardResults)] (waitAll) WATCH OUT, set waiter onto... waitQueue.head = (nil)
[0x7fce8ebfd700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1799][group(0x55e4c18fc5b0,discardResults)] (waitAll) waitAll, marked waiting status = TaskGroupStatus{ C:n W:y P:1 0100000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1159][group(0x55e4c18fc5b0,discardResults)] (offer) offer, completedTask:0x7fce88029c70, error:1, status:TaskGroupStatus{ C:n W:y P:1 0100000000000000000000000000000000000000000000000000000000000001 }
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1167][group(0x55e4c18fc5b0,discardResults)] (offer) offer, complete, status afterComplete:TaskGroupStatus{ C:n W:y P:0 0100000000000000000000000000000000000000000000000000000000000000 }
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1256][group(0x55e4c18fc5b0,discardResults)] (resumeWaitingTask) resume waiting task = 0x55e4c18fc290, alreadyDecremented:1, error:1, complete with = 0x7fce88029c70
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1259][group(0x55e4c18fc5b0,discardResults)] (resumeWaitingTask) resumeWaitingTask, attempt CAS, waiting task = 0x55e4c18fc290, waitQueue.head = 0x55e4c18fc290, error:1, complete with = 0x7fce88029c70
[0x7fce8f5fe700] [/home/build-user/swift/stdlib/public/Concurrency/TaskGroup.cpp:1296][group(0x55e4c18fc5b0,discardResults)] (resumeWaitingTask) resume waiting DONE, task = 0x55e4c18fc290, backup = 0x55e4c18fc290, complete with = 0x7fce88029c70, status = TaskGroupStatus{ C:y W:y P:0 1100000000000000000000000000000000000000000000000000000000000000 }
pure virtual method called
terminate called without an active exception
/home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Concurrency/Runtime/Output/async_taskgroup_discarding.swift.script: line 1: 25881 Aborted                 (core dumped) /usr/bin/env LD_LIBRARY_PATH='/home/build-user/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux' /home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Concurrency/Runtime/Output/async_taskgroup_discarding.swift.tmp/a.out

--

Unable to reproduce this issue on a linux host 👎 Haven been trying all day to find some reason for this.

ktoso · 2023-01-24T09:02:58Z

@swift-ci please smoke test

ktoso · 2023-01-24T09:21:34Z

Managed to reproduce the other occasional issue, with debug output:

[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:847][group(0x559fad1a15f0)] (swift_taskGroup_initializeWithFlagsImpl) create group; flags: isDiscardingResults=1
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:631][group(0x559fad1a15f0,discardResults)] (statusAddPendingTaskRelaxed) addPending, after = TaskGroupStatus{ C:n W:n P:1 0000000000000000000000000000000000000000000000000000000000000001 }
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:631][group(0x559fad1a15f0,discardResults)] (statusAddPendingTaskRelaxed) addPending, after = TaskGroupStatus{ C:n W:n P:2 0000000000000000000000000000000000000000000000000000000000000010 }
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1728][group(0x559fad1a15f0,discardResults)] (waitAll) waitAll, bodyError = (nil), status = TaskGroupStatus{ C:n W:n P:2 0000000000000000000000000000000000000000000000000000000000000010 }
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1743][group(0x559fad1a15f0,discardResults)] (waitAll) waitAll, status = TaskGroupStatus{ C:n W:n P:2 0000000000000000000000000000000000000000000000000000000000000010 }
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1792][group(0x559fad1a15f0,discardResults)] (waitAll) WATCH OUT, set waiter onto... waitQueue.head = (nil)
[0x7f032a0a9640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1799][group(0x559fad1a15f0,discardResults)] (waitAll) waitAll, marked waiting status = TaskGroupStatus{ C:n W:y P:2 0100000000000000000000000000000000000000000000000000000000000010 }
[0x7f032b8ac640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1159][group(0x559fad1a15f0,discardResults)] (offer) offer, completedTask:0x7f031c00ed00, error:0, status:TaskGroupStatus{ C:n W:y P:2 0100000000000000000000000000000000000000000000000000000000000010 }
[0x7f032b8ac640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1167][group(0x559fad1a15f0,discardResults)] (offer) offer, complete, status afterComplete:TaskGroupStatus{ C:n W:y P:1 0100000000000000000000000000000000000000000000000000000000000001 }
[0x7f032a8aa640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1159][group(0x559fad1a15f0,discardResults)] (offer) offer, completedTask:0x7f031c00e820, error:1, status:TaskGroupStatus{ C:n W:y P:1 0100000000000000000000000000000000000000000000000000000000000001 }
[0x7f032a8aa640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1167][group(0x559fad1a15f0,discardResults)] (offer) offer, complete, status afterComplete:TaskGroupStatus{ C:n W:y P:0 0100000000000000000000000000000000000000000000000000000000000000 }
[0x7f032a8aa640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1256][group(0x559fad1a15f0,discardResults)] (resumeWaitingTask) resume waiting task = 0x559fad1a1290, alreadyDecremented:1, error:1, complete with = 0x7f031c00e820
[0x7f032a8aa640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1259][group(0x559fad1a15f0,discardResults)] (resumeWaitingTask) resumeWaitingTask, attempt CAS, waiting task = 0x559fad1a1290, waitQueue.head = 0x559fad1a1290, error:1, complete with = 0x7f031c00e820
[0x7f032a8aa640] [/home/ktoso/swift-project/swift/stdlib/public/Concurrency/TaskGroup.cpp:1296][group(0x559fad1a15f0,discardResults)] (resumeWaitingTask) resume waiting DONE, task = 0x559fad1a1290, backup = 0x559fad1a1290, complete with = 0x7f031c00e820, status = TaskGroupStatus{ C:y W:y P:0 1100000000000000000000000000000000000000000000000000000000000000 }
main/async_taskgroup_discarding.swift:56: Fatal error: (iteration:33) expected error to be re-thrown, got: 12
Current stack trace:
0    libswiftCore.so                    0x00007f032c5df890 _swift_stdlib_reportFatalErrorInFile + 112
1    libswiftCore.so                    0x00007f032c1d3892 <unavailable> + 1517714
2    libswiftCore.so                    0x00007f032c1d36e6 <unavailable> + 1517286

but I can't understand how that's possible; we never hit any "store the result" codepath in the first offer, so how does the 12 end up in the result...

ktoso · 2023-01-24T11:19:29Z

preset=asan
@swift-ci Please test with preset macOS

ktoso · 2023-01-24T11:19:42Z

@swift-ci Please smoke test linux

ktoso · 2023-01-24T12:57:43Z

@swift-ci Please smoke test

ktoso · 2023-01-24T14:09:39Z

@swift-ci Please smoke test macOS

ktoso · 2023-01-24T14:52:14Z

Seems linux is passing with the latest rework of waitAll body -- avoiding a race on context mutation. Let's give it another spin though.

Yeah all those issues can be tracked back to an use-after-free I found and finally fixed now.

ktoso · 2023-01-24T14:52:26Z

@swift-ci Please smoke test linux

…f lock

ktoso · 2023-01-25T02:32:49Z

@swift-ci Please smoke test

ktoso · 2023-01-25T02:40:50Z

Unlocking one more test

@swift-ci Please smoke test

ktoso · 2023-01-25T02:42:03Z

Also picked over the overflow protection we did way back then in #62271

I believe we're finally golden here, let's give it another test run and I'll pick forward.

ktoso · 2023-01-25T09:53:07Z

@swift-ci please test

ktoso · 2023-01-25T14:28:56Z

ARM_ARCH_7K compilation fun™, fixed now.

ktoso · 2023-01-25T14:29:08Z

@swift-ci please test

ktoso · 2023-01-26T13:59:21Z

@swift-ci please smoke test windows

ktoso requested review from FranzBusch, DougGregor, shahmishal and beccadax January 13, 2023 13:58

ktoso mentioned this pull request Jan 13, 2023

[Concurrency] uncomment isCancelled override decl #63007

Merged

ktoso added the concurrency Feature: umbrella label for concurrency language features label Jan 13, 2023

ktoso mentioned this pull request Jan 13, 2023

[5.8][Concurrency] Fix too optimistic TaskGroup bail-out-when-empty #63017

Merged

ktoso force-pushed the wip-waitAll-race branch 2 times, most recently from 642dc77 to 13de654 Compare January 16, 2023 09:36

ktoso force-pushed the wip-waitAll-race branch from 47b76c1 to b34b67c Compare January 16, 2023 14:03

ktoso force-pushed the wip-waitAll-race branch from 332c1bd to c49bd0a Compare January 17, 2023 04:51

ktoso force-pushed the wip-waitAll-race branch from 7286220 to c87ebdc Compare January 17, 2023 09:03

ktoso commented Jan 17, 2023

View reviewed changes

ktoso mentioned this pull request Jan 17, 2023

Disable async_taskgroup_void_neverConsumingTasks.swift #63064

Merged

ktoso removed the request for review from shahmishal January 18, 2023 13:10

[Concurrency] Fix too optimistic bail-out-when-empty, discarding group

ccc02d0

may need to emit an error out of such waitAll attempt, if a previous error was already stored.

explicitly declare virtual destructors

27de643

ktoso force-pushed the wip-waitAll-race branch from 945cd59 to 8525a45 Compare January 24, 2023 12:56

detach child task after filling result; dont mutate context outside o…

028ab32

…f lock

ktoso force-pushed the wip-waitAll-race branch from 8525a45 to 2c7038b Compare January 25, 2023 02:13

Add pending count overflow protection to TaskGroup

e75ef10

ktoso force-pushed the wip-waitAll-race branch from 2c7038b to e75ef10 Compare January 25, 2023 02:26

unlock async_taskgroup_void_neverConsumingTasks again rdar://104332560

d6ed553

missing include on windows for error reporting

84a0cba

fix ARM_ARCH_7K workaround snippet

23699b0

ktoso force-pushed the wip-waitAll-race branch from ead2375 to 23699b0 Compare January 25, 2023 14:28

ktoso merged commit 99fb37f into swiftlang:main Jan 27, 2023

ktoso deleted the wip-waitAll-race branch January 27, 2023 10:00

[Concurrency] Fix too optimistic TaskGroup bail-out-when-empty, #63016

[Concurrency] Fix too optimistic TaskGroup bail-out-when-empty, #63016

Uh oh!

Conversation

ktoso commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 13, 2023

Uh oh!

ktoso commented Jan 13, 2023

Uh oh!

ktoso commented Jan 13, 2023

Uh oh!

ktoso commented Jan 16, 2023

Uh oh!

ktoso commented Jan 16, 2023

Uh oh!

ktoso commented Jan 16, 2023

Uh oh!

ktoso commented Jan 17, 2023

Uh oh!

ktoso commented Jan 17, 2023

Uh oh!

ktoso commented Jan 17, 2023

Uh oh!

ktoso Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso commented Jan 18, 2023

Uh oh!

ktoso commented Jan 19, 2023

Uh oh!

ktoso commented Jan 19, 2023

Uh oh!

ktoso commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 24, 2023

Uh oh!

ktoso commented Jan 25, 2023

Uh oh!

ktoso commented Jan 25, 2023

Uh oh!

ktoso commented Jan 25, 2023

Uh oh!

ktoso commented Jan 25, 2023

Uh oh!

ktoso commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Jan 25, 2023

Uh oh!

ktoso commented Jan 13, 2023 •

edited

Loading

ktoso commented Jan 24, 2023 •

edited

Loading

ktoso commented Jan 24, 2023 •

edited

Loading

ktoso commented Jan 24, 2023 •

edited

Loading

ktoso commented Jan 24, 2023 •

edited

Loading

ktoso commented Jan 24, 2023 •

edited

Loading

ktoso commented Jan 25, 2023 •

edited

Loading