Check if passing of cancellation token to `SqlTableChangeMonitor` class makes sense

[JatinSanghvi]

This issue tracks the fix to address the code review comment in #226.

This seems like something we should be doing

Originally posted by @Charles-Gagnon in #226 (comment)

[AmeyaRele]

Looked into this, with reference to other extensions.
The CancellationToken in StartAsync method is limited to that method in all of the extensions. TimerTrigger, CosmosDBTrigger
The cancellation token used during actual execution is always a different one.
In SqlTableChangeMonitor there are 3 cancellationTokenSource instantiated. One for RunChangeConsumptionLoopAsync, RunLeaseRenewalLoopAsync and for function executor.

Also, none of the calls made in SqlTableChangeMonitor constructor uses CancellationToken. Based on this the CancellationToken does not need to be passed to SqlTableChangeMonitor. Discussed this with @JatinSanghvi as well.

Would appreciate your thoughts on this @Charles-Gagnon.

[Charles-Gagnon]

Gotcha, yeah that makes sense. Looks like we're handling the StopAsync stuff correctly too - although if you could make sure you go through and look at that it'd be good to have some more eyes verifying that all the connections are being closed properly.

One thing that would be nice is to update

azure-functions-sql-extension/src/TriggerBinding/SqlTableChangeMonitor.cs

Line 159 in 8c01883

this._cancellationTokenSourceCheckForChanges.Cancel();

to have a comment saying that the lease renewal loop will be stopped once the CheckForChanges loop finishes. (It might be cleaner to cancel both there instead, but not something I see as a huge problem) just to make sure anyone looking at the code isn't confused about why we only cancel one of the sources.

[AmeyaRele]

One thing that would be nice is to update

azure-functions-sql-extension/src/TriggerBinding/SqlTableChangeMonitor.cs

Line 159 in 8c01883

this._cancellationTokenSourceCheckForChanges.Cancel();

to have a comment saying that the lease renewal loop will be stopped once the CheckForChanges loop finishes. (It might be cleaner to cancel both there instead, but not something I see as a huge problem) just to make sure anyone looking at the code isn't confused about why we only cancel one of the sources.

Created PR for this, closing the issue.