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.

Sign up for GitHub

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

Fix Async.Parallel stack overflow on cancellation of ~2000 uncompleted #13186

Merged
merged 3 commits into from
May 24, 2022
Merged

Fix Async.Parallel stack overflow on cancellation of ~2000 uncompleted #13186

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34e13b5
Fix Async.Parallel stack overflow on cancellation of ~2000 uncomplete…
dsyme May 23, 2022
0aadd73
Update AsyncModule.fs
dsyme May 23, 2022
d9935fa
Update AsyncModule.fs
dsyme May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/FSharp.Core/async.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1470,21 +1470,21 @@ type Async =
if innerCTS.Token.IsCancellationRequested then
let cexn = OperationCanceledException (innerCTS.Token)
recordFailure (Choice2Of2 cexn) |> unfake
worker trampolineHolder |> unfake
worker trampolineHolder
else
let taskCtxt =
AsyncActivation.Create
innerCTS.Token
trampolineHolder
(fun res -> recordSuccess j res |> unfake; worker trampolineHolder)
(fun edi -> recordFailure (Choice1Of2 edi) |> unfake; worker trampolineHolder)
(fun cexn -> recordFailure (Choice2Of2 cexn) |> unfake; worker trampolineHolder)
(fun res -> recordSuccess j res |> unfake; worker trampolineHolder |> fake)
(fun edi -> recordFailure (Choice1Of2 edi) |> unfake; worker trampolineHolder |> fake)
(fun cexn -> recordFailure (Choice2Of2 cexn) |> unfake; worker trampolineHolder |> fake)
computations.[j].Invoke taskCtxt |> unfake
fake()

for x = 1 to maxDegreeOfParallelism do
let trampolineHolder = TrampolineHolder()
trampolineHolder.QueueWorkItemWithTrampoline (fun () ->
worker trampolineHolder)
worker trampolineHolder |> fake)
|> unfake

fake()))
Expand Down
12 changes: 12 additions & 0 deletions tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,3 +764,15 @@ type AsyncModule() =
lock gate <| fun () -> printfn "Unhandled exception: %s" exn.Message
lock gate <| fun () -> printfn "Semaphore count available: %i" semaphore.CurrentCount
Assert.AreEqual(acquiredCount, releaseCount)

[<Fact>]
member _.``Async.Parallel blows stack when cancelling many`` () =
Copy link

@bartelink bartelink May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
member _.``Async.Parallel blows stack when cancelling many`` () =
// versions 4.7 - 6.0.4 impl didn't tail recurse as intended and hence can blow the stack given ~1600 excess computations at point of cancellation
member _.``Async.Parallel handles early cancelation of large computation streams safely`` () =

let gen (i : int) = async {
if i <> 0 then do! Async.Sleep i
else return failwith (string i) }
let count = 1800
Copy link

Choose a reason for hiding this comment

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

if this is to function as a regression test in perpetuity and/or across platforms/processors, perhaps doubling this empirically derived value might make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doubled!

let comps = Seq.init count gen
let result = Async.Parallel(comps, 16) |> Async.Catch |> Async.RunSynchronously
match result with
| Choice2Of2 e -> ()
dsyme marked this conversation as resolved.
Show resolved Hide resolved
| x -> failwithf "unexpected %A" x