Dispose the cancellationTokenSouce

[charlieknoll]

Dispose the cancellationTokenSouce in order to prevent it from firing after Task is complete

[coveralls]

Coverage remained the same at 94.309% when pulling 951c90b on charlieknoll:patch-1 into a42a05d on jamesmanning:master.

[EamonNerbonne]

Disposing CancellationTokenSouce is unlikely to be relevant here (see source if you want) but it's nice to be clean (especially in docs); sure! If you're doing this to keep up good habits, then I'd suggest a using instead; that's more conventional, and "safer" in that it works in the event of exceptions.

It's a little unfortunate that the async apis are full of Disposables that are conventionally non-disposed, but that's just the way it works.

[jamesmanning]

@EamonNerbonne I ended up committing a using() on master for this as you suggest, but I wanted to sanity check one thing. You said "unlikely to be relevant" and I'm not disagreeing with you, but I wanted to make sure I understood your intent on "relevant" here. The Dispose(true) path disposes m_timer AFAICT which seems relevant since m_timer was created in this use case as we created the CTS with a timeout.

For the call stack, we're calling ctor(TimeSpan) which calls InitializeWithTimer which creates the Timer instance. Again, I'm not disagreeing, but since it looks relevant based on a cursory glance, it felt like I was likely missing something so I wanted to better understand your feedback. 😄

Thanks @EamonNerbonne !

[EamonNerbonne]

Timer has a finalizer which will clean it up. Not just that; the overhead for leaving a timer alive that's never reached appears to 0 or close to that; and if it is fired; this isn't a repeating timer: so firing it implies deleting it.

In essence: this all appears to work as common sense might suggest: the sole meaningful consequence of failing to unregister a timer is that it will still fire (and that implies the overhead of cancelling a token nobody is watching). So you can avoid the timer firing once; but that's hardly a perf nightmare. And the finalizer might even kick in before the timer anyhow.

Given the scope (running processes), these things aren't going to be relevant performance issues; they might even be so small as to be unmeasurable.

[jamesmanning]

@EamonNerbonne that makes sense, thanks for the explanation! 👍