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

Some AsyncMemoize fixes #18074

Merged
merged 6 commits into from
Dec 2, 2024
Merged

Some AsyncMemoize fixes #18074

merged 6 commits into from
Dec 2, 2024

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Nov 27, 2024

Description

AsyncMemoize tests became flaky again because of recent changes, for example: https://dev.azure.com/dnceng-public/public/_build/results?buildId=881078&view=ms.vss-test-web.build-test-results-tab&runId=23026540&resultId=100189&paneView=debug

The problem was a not atomic handling of request count. It was possible to cancel the request mid-flight in such a way that the finally block decreasing request count never run.

Checklist

  • Verified locally by running thousands of iterations concurrently.

Copy link
Contributor

github-actions bot commented Nov 27, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@majocha majocha mentioned this pull request Nov 27, 2024
1 task
@majocha majocha marked this pull request as ready for review November 27, 2024 20:24
@majocha majocha requested a review from a team as a code owner November 27, 2024 20:24
@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Nov 28, 2024
@majocha
Copy link
Contributor Author

majocha commented Nov 29, 2024

Yeah, this returns an async computation that decreases request count in finally. But the increase is outside of the try block now.

| Initial computation ->
let cts = new CancellationTokenSource()
let work = Async.StartAsTask(computation, cancellationToken = cts.Token)
state <- Running (computation, work, cts, 1)
detachable work

Benign in real usage but can be tripped in tests causing job cancellation to not happen.

@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thank you :)

@psfinaki psfinaki merged commit 60d686c into dotnet:main Dec 2, 2024
33 checks passed
@0101
Copy link
Contributor

0101 commented Dec 2, 2024

Got a stress test failure with this fix already included :/

https://dev.azure.com/dnceng-public/public/_build/results?buildId=883746&view=ms.vss-test-web.build-test-results-tab&runId=23109844&resultId=100187&paneView=debug
( Test timed out - most likely deadlocked)

@majocha
Copy link
Contributor Author

majocha commented Dec 2, 2024

I'll try to stress test the stress test locally and reproduce it. The timeout in stress-test is very generous, it doesn't look like a fluke.

@majocha
Copy link
Contributor Author

majocha commented Dec 2, 2024

I can't repro the deadlock but after some 1.5k iterations I got once:

    ---- System.Exception : Seed 57146328 failed on iteration 30: System.AggregateException: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
   --- End of inner exception stack trace ---
   at <StartupCode$FSharp-Compiler-Service>.$AsyncMemoize.catchHandler@1[t](Exception exn) in E:\repos\fsharp\src\Compiler\Facilities\AsyncMemoize.fs:line 54
   at <StartupCode$FSharp-Compiler-Service>.$AsyncMemoize.clo@42-31.Invoke(Exception x)
   at Microsoft.FSharp.Control.AsyncPrimitives.CallFilterThenInvoke[T](AsyncActivation`1 ctxt, FSharpFunc`2 filterFunction, ExceptionDispatchInfo edi) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 547
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 114
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at CompilerService.AsyncMemoize.loop@323-8.MoveNext() in E:\repos\fsharp\tests\FSharp.Compiler.ComponentTests\CompilerService\AsyncMemoize.fs:line 338
---> (Inner Exception #0) System.AggregateException: One or more errors occurred. ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
---> (Inner Exception #0) System.Threading.Tasks.TaskCanceledException: A task was canceled.<---
<---

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants