Observe canceled tasks in WithCancellation

[chkimes]

After timeout, WithCancellation throws an exception meaning the return await task.ConfigureAwait(false); line is never invoked. If this task is faulted (likely after it was canceled) then it will not be observed and exceptions will be thrown on the finalizer thread.

[MichaCo]

Hi @chkimes, do you have some example of how to reproduce the "bad" behavior? Just interested.

I'll merge this anyways

[MichaCo]

I actually had to revert your PR because it was pushing to the wrong branch (master) and not dev.

[chkimes]

Should I re-create a PR targeting dev or have you handled that?

The simplest way to re-create the behavior would be to set up a handler for unhandled exceptions to observe the exception on the finalizer thread, set up a task that throws an exception after N seconds, use WithCancellation to time out in <N seconds, then force garbage collection. Make sure not to await the throwing task, otherwise it won't be unobserved before garbage collection.

[chkimes]

We observed this when calls to improperly configured nameservers were timing out, and our unhandled exception handler was writing this exception message to our logs:

A Task's exception(s) were not observed either by Waiting on the Task or accessing its Exception property. As a result, the unobserved exception was rethrown by the finalizer thread.

[MichaCo]

I'll add the change to the dev branch with some other things, you don't have to create another PR, thanks ;)

And thanks for the explanation.

I'm wondering if, in general, it is even a good idea to leave the task "running".

I guess if I would await the task itself, after the cancellation hit, would also "solve" that problem?

[chkimes]

I'm wondering if, in general, it is even a good idea to leave the task "running".

I think that by using this method, we're accepting that the task cannot be canceled by any other means (e.g. UdpClient methods) - so I'm not sure there's any option for stopping it from "running" and if there were then it would be better to use that option instead of this method.

edit: regarding general cleanliness of .NET, as long as the resources that are referencing this task are cleaned up then it will eventually be garbage collected, so there's nothing wrong with leaving it around in the "running" state if it gets orphaned.

I guess if I would await the task itself, after the cancellation hit, would also "solve" that problem?

That would solve this particular issue but would introduce a new problem, such as if the task never completes for some reason.

[MichaCo]

Yeah I agree with not having to await, or in this case better not to, to not block everything. That being said, I actually found a bug. I never passed the token, which would cancel after the set timeout, to the actual query method. That means that the task couldn't have been canceled via inspecting the token and such...

see #124

I also figured that I can remove the custom action callback and just use a CancellationTokenRegistration insterad.

[chkimes]

I also figured that I can remove the custom action callback and just use a CancellationTokenRegistration insterad.

Cool! That's a lot cleaner - and I'm sure easier for people to understand. It took a while of going back and forth for me to realize what the original code was doing :)