Dispose ConfigurationReloadToken

[HaoK]

Merge of #588

[Tratcher]

This is dangerous and unnecessary. There's no need to dispose a CTS that was already cancelled. Also, doing so would make RegisterChangeCallback throw OED instead of invoking the cancellation callback inline. We've had a lot of problems with this race elsewhere in the stack and have removed the disposal calls.

A more interesting question is do you ever dispose the ConfigurationReloadToken in the event that it won't fire?

[ajcvickers]

@HaoK So based on Chris's comments, it sounds like we shouldn't do this.

[HaoK]

Yeah I guess the issue is that we shouldn't dispose the underlying cancellation token after it fires, but we maybe should dispose the ConfigurationReloadToken as part of ConfigurationRoot's dispose:
https://github.com/aspnet/Extensions/blob/master/src/Configuration/Config/src/ConfigurationRoot.cs#L124

Thoughts?

[Tratcher]

Those roots would only be disposed at the end of the application, no? Yes that'd be a good idea, but I don't see how it ties back to the original bug report.

[HaoK]

I'm not sure what you are referring to in the original bug, you mean "Possible CTS leak in ConfigurationReloadToken / ConfigurationRoot" ? If its not safe to dispose the CTS after firing, then the only thing we can do is dispose it when the root is disposed.

[Tratcher]

They implied there was a memory accrual for non-disposed tokens during the lifetime of the app. It doesn't look that that was ever confirmed.

[HaoK]

Configuration leaking was addressed via #861 so yeah that was reported in other issues

[HaoK]

Closing this for now, issue remains open for tracking/discussion