Skip to content

AsyncLazy bugfix - #18101

Merged
0101 merged 7 commits into
dotnet:mainfrom
majocha:asynclazy-bugs
Dec 6, 2024
Merged

AsyncLazy bugfix #18101
0101 merged 7 commits into
dotnet:mainfrom
majocha:asynclazy-bugs

Conversation

@majocha

Copy link
Copy Markdown
Contributor

Description

New code, new bugs:
#18074 (comment)

It is incredibly easy to write buggy code when concurrent mutations of state are involved. 😐
This change is to contain the mutation to just a single function with lock.
Fixes this rare edge case with unawaited completion in AsyncLazy:
#18074 (comment)

@github-actions

github-actionsBot commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

⚠️ 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
Copy Markdown
ContributorAuthor

@0101, I couldn't repro locally this timeout in stress-test you pointed out:
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
but I fixed another bug, so maybe possibly it will help with it?

Maybe it would be better to use --blame-hang-timeout in the CI instead, with some generous timeout, like 5 minutes? In case of a genuine deadlock or hang we'd at least get a dump.

@0101

0101 commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

@0101, I couldn't repro locally this timeout in stress-test you pointed out: 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 but I fixed another bug, so maybe possibly it will help with it?

Let's try!

Maybe it would be better to use --blame-hang-timeout in the CI instead, with some generous timeout, like 5 minutes? In case of a genuine deadlock or hang we'd at least get a dump.

Yeah that sounds like a good idea. Hopefully we don't have any actually long running test. Well if we do we'll find out.

@majocha
majocha marked this pull request as ready for review December 4, 2024 17:11
@majocha
majocha requested a review from a team as a code ownerDecember 4, 2024 17:11

@psfinakipsfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! We are getting there... :)

@01010101 added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Dec 6, 2024
0101
0101 approved these changes Dec 6, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NO_RELEASE_NOTESLabel 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

@majocha@0101@psfinaki