Skip to content

Conversation

@majocha
Copy link
Contributor

@majocha majocha commented Jan 7, 2025

Description

Make the logic around cancelling requests more straightforward.
Explicitly handle TaskCanceledException to prevent AsyncLazy from caching it.

Fixes #18209

Checklist

  • The bug caused AsyncMemoize stress test to fail sometimes. Tested by running 1000 iterations and making sure it no longer reproduce locally.
  • Release notes entry updated: Experimental so maybe release notes not needed.

@majocha majocha requested a review from a team as a code owner January 7, 2025 18:42
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025

⚠️ 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
Copy link
Contributor Author

majocha commented Jan 7, 2025

I was hoping to make this code as clear and straightforward as possible, but I see now there is endless potential for bugs. On the brighter note, we already have another implementation with very similar functionality. If need arises, GraphNode can serve as almost a drop-in replacement for AsyncLazy. Now when this is factored out from AsyncMemoize it should not be hard to swap implementations if we want to. Hopefully it won't be needed.

@0101 0101 added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jan 9, 2025
@psfinaki
Copy link
Contributor

psfinaki commented Jan 9, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vzarytovskii vzarytovskii requested a review from Copilot January 9, 2025 15:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (6)
  • src/Compiler/Facilities/AsyncMemoize.fs: Language not supported
  • src/Compiler/Facilities/AsyncMemoize.fsi: Language not supported
  • tests/ILVerify/ilverify_FSharp.Compiler.Service_Debug_net9.0.bsl: Language not supported
  • tests/ILVerify/ilverify_FSharp.Compiler.Service_Debug_netstandard2.0.bsl: Language not supported
  • tests/ILVerify/ilverify_FSharp.Compiler.Service_Release_net9.0.bsl: Language not supported
  • tests/ILVerify/ilverify_FSharp.Compiler.Service_Release_netstandard2.0.bsl: Language not supported

@psfinaki
Copy link
Contributor

psfinaki commented Jan 9, 2025

Thanks @majocha. Yeah it's about progress, not perfection here with async states. As long as a change somewhat improves the situation, it's all good.

@psfinaki psfinaki merged commit b204f87 into dotnet:main Jan 9, 2025
33 of 34 checks passed
@majocha majocha deleted the asynclazy-bug branch January 9, 2025 19:00
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.

Bug in AsyncLazy handling of cancellation

3 participants