Skip to content

Observe canceled tasks in WithCancellation#121

Merged
MichaConrad merged 2 commits into
MichaCo:masterfrom
chkimes:patch-1
Jun 12, 2021
Merged

Observe canceled tasks in WithCancellation#121
MichaConrad merged 2 commits into
MichaCo:masterfrom
chkimes:patch-1

Conversation

@chkimes

@chkimes chkimes commented Jun 9, 2021

Copy link
Copy Markdown

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.

@MichaConrad

Copy link
Copy Markdown
Collaborator

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

I'll merge this anyways

@MichaConrad MichaConrad added this to the 1.5.0 milestone Jun 12, 2021
@MichaConrad MichaConrad merged commit b2c9ec1 into MichaCo:master Jun 12, 2021
MichaConrad pushed a commit that referenced this pull request Jun 12, 2021
MichaConrad pushed a commit that referenced this pull request Jun 12, 2021
@MichaConrad

Copy link
Copy Markdown
Collaborator

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

@chkimes

chkimes commented Jun 12, 2021

Copy link
Copy Markdown
Author

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

chkimes commented Jun 12, 2021

Copy link
Copy Markdown
Author

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.

@MichaConrad

Copy link
Copy Markdown
Collaborator

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

chkimes commented Jun 12, 2021

Copy link
Copy Markdown
Author

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.

@MichaConrad

MichaConrad commented Jun 12, 2021

Copy link
Copy Markdown
Collaborator

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.

MichaConrad pushed a commit that referenced this pull request Jun 12, 2021
Fixed one bug where the wrong cancellation token was passed to the async query impl...
Removed the custom cancellation callback to dispose UdpClient for example, can now use the cancellation token + a callback registration.
Includes other fix from #121
@chkimes

chkimes commented Jun 12, 2021

Copy link
Copy Markdown
Author

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 :)

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.

2 participants