Fix goroutine leak on closing

[ItalyPaleAle]

It appears that these goroutines do not receive the context, so the scheduler keeps running even after shutdown

[ItalyPaleAle]

If someone cancels the context after StartNonBlocking returns they'll end up cancelling their already started processor.

Got it, although that's how contexts are supposed to be used, as a way to cancel background processes.

I know that a "track2" of this SDK is in the works so perhaps spending too much time optimizing this isn't even worth it at this point.

[richardpark-msft]

If someone cancels the context after StartNonBlocking returns they'll end up cancelling their already started processor.

Got it, although that's how contexts are supposed to be used, as a way to cancel background processes.

Yeah, it's a good point. I can see it both ways - do you expect to keep the context to still apply long past the call being "complete" though? It seems like it's supposed to be considered safe to just cancel it after the function call returns, but in this case you'd still end up cancelling the .Run().

I know that a "track2" of this SDK is in the works so perhaps spending too much time optimizing this isn't even worth it at this point.

This is true, but your point is 100% valid so I'd like to make sure, if we have do something similar, that our design makes sense to other Go people.

[richardpark-msft]

@ItalyPaleAle, all of what I wrote above only applies to the non-blocking version of the call. The blocking version makes sense to me.

[ItalyPaleAle]

do you expect to keep the context to still apply long past the call being "complete" though?

IMHO, yes. Usually, when a method accepts a context and runs in background, the context is the cancellation signal.

[jhendrixMSFT]

I'm also a bit worried that if we change the behavior now it will break somebody that depends on it.

Given that a track 2 replacement is on the way, I'd prefer if we just document this behavior instead. We'll improve the API for track 2. Does that work @ItalyPaleAle for you?

[ItalyPaleAle]

Yes that makes sense.

In the meanwhile, is there a way to fix the goroutine leak without breaking changes? For example, adding another context that is started inside StartNonBlocking and stopped on Close?

[jhendrixMSFT]

Agreed that should be fixed. I made some changes to your PR. Let me know if it works for you.

[ItalyPaleAle]

@jhendrixMSFT thanks for the changes! (To my limited knowledge) LGTM :)

[jhendrixMSFT]

If @richardpark-msft approves I'll get it merged and tagged.