Skip to content

Avoid unobserved task exception in AsyncAtomicFactory #686

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

Merged
merged 9 commits into from
Jun 15, 2025

Conversation

advdotnet
Copy link
Contributor

Garbage collecting an AsyncAtomicFactory instance can cause an unobserved task exception when a value factory threw.
This is due to a TaskCompletionSource.SetException call in the AsyncAtomicFactory.Initializer class and its task's exception is never observed.

@bitfaster
Copy link
Owner

bitfaster commented Jun 12, 2025

Thanks for this - I ran the PR build on your change yesterday and your test fails on .NET6 but not Fwk. I started to look but was paged at work so didn't yet get to understand what is wrong.

Edit: now I see you pushed more changes - re-running it.

@coveralls
Copy link

coveralls commented Jun 12, 2025

Coverage Status

coverage: 99.102% (-0.07%) from 99.168%
when pulling 41e7272 on advdotnet:main
into aefdf90 on bitfaster:main.

… as well

Remove test in AsyncAtomicFactoryTests again
@advdotnet
Copy link
Contributor Author

Thanks for this - I ran the PR build on your change yesterday and your test fails on .NET6 but not Fwk.

Edit: now I see you pushed more changes - re-running it.

I suspected that it failed in the first run due to another unobserved task exception from other tests. Indeed, there was the same pattern present in the ScopedAsyncAtomicFactory.Initializer. After fixing that, I did not encounter any unobserved task exceptions when running the unit tests. However, the test is removed now as it does not provide much value anyway. Let me know if you want it added again, then I could throw and check for a dedicated exception type that will only be used in that test.

@bitfaster
Copy link
Owner

I think your change will break the scenario where two threads are waiting on the same underlying task, there was no test case for this so I added it here: #687

I merged this into your changes so the tests should now run to verify.

@bitfaster
Copy link
Owner

I tried out the scenario using your test - although it is cheesy, I think best approach is to simply await the task to throw the exception inside AsyncAtomicFactory (see #688). Using that approach, it is possible to support both avoiding the unobserved task exception and support two or more threads waiting on the same task (the scenario I added the test for).

If you want to merge the await method here with your test and complete this yourself that would be great - I think it is good to have the test running on changes, that way I can't screw it up again.

@advdotnet
Copy link
Contributor Author

Excellent, done. Also logged the actual unobserved task exception so one could see where it came from.

@bitfaster bitfaster merged commit 2e420f0 into bitfaster:main Jun 15, 2025
13 checks passed
@bitfaster
Copy link
Owner

Thanks for this, merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants