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

AsyncHelper.WaitForCompletion leaks unobserved exceptions #688

Closed
jm771 opened this issue Aug 13, 2020 · 3 comments
Closed

AsyncHelper.WaitForCompletion leaks unobserved exceptions #688

jm771 opened this issue Aug 13, 2020 · 3 comments

Comments

@jm771
Copy link
Contributor

jm771 commented Aug 13, 2020

To reproduce

I have a reproduction of the issue along with a possible fix here:
master...jm771:ReproUnobservedException

Bug description

WaitForCompletion waits for a task to complete with a timeout, and allows you to specify a handler for the timeout case. However, once the timeout has occurred it ditches the task, which means if that task later completes with whatever error caused it to timeout, the exception will be unobserved, and the UnobservedTaskException event will fire.

As AsyncHelper is internal, the bug wasn't experienced directly, but certain timeout conditions in Sql connections would lead to unobserved exceptions. My former employer had a policy of no unobserved exceptions, and so had a handler that killed the application when they occurred. We worked around this at the time by changing the handler to ignore this type of exception, and it's only now I'm finding time to file a proper bug report.

@karinazhou
Copy link
Member

karinazhou commented Aug 13, 2020

Hi @jm771 ,

This is a good catch. I will test your branch locally to validate your changes. Meanwhile, please feel free to create the PR so we can review it properly.

@jm771
Copy link
Contributor Author

jm771 commented Aug 14, 2020

I have raised a PR here: #692

@cheenamalhotra cheenamalhotra added this to To do in SqlClient v2.1 via automation Aug 19, 2020
@karinazhou
Copy link
Member

Hi @jm771 ,

We are closing this issue because your PR has been merged.

Thanks,

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

No branches or pull requests

2 participants