Proper cancellation of status checks

[marcusoffesson]

If a status check was canceled while it was running, it actually cancelled an old timeout identifier which in turn caused status check to go rouge (they continued forever, consuming memory in each iteration).

Any ideas of how to test this in a controlled environment is highly appreciated!

[wennmo]

Nice!

[peol]

For testing—perhaps mock this.statusFetcher.fetch to resolve a promise after e.g. 5 seconds, and before that do stopStatusChecks() and ensure after 5 seconds that the loop is no longer running (by e.g. checking this.statusFetcher.fetch calls are still 1).

[peol]

I'd rather have this check on line 74 instead (e.g. only do another setTimeout if we are still running)—for clarity's sake.

[janmiderback]

I think I agree 🙂

[marcusoffesson]

I can move it, but be advised, it will lead to unnecessary health/metrics checks if cancellation is made in between status checks (which should be the normal scenario when we don't have recurring engine crashes).

[janmiderback]

👍 One comment on a convention.

[janmiderback]

Are we moving away from the convention to have "private" methods not part of the class definition?
If so, we should align all code with this.

[marcusoffesson]

So that's the reason!
Me and @peol discussed this and I did not know the reason for having it outside the class. Anyway, one argument for having it part of the class is to avoid having to use bind() which creates a new copy of the function every time it's called if I'm correctly informed.

[janmiderback]

I'm fine with putting them in the class def, but we should align.
Also, it would be very nice if we had some convention for naming "private" methods. I have seen _ suffix but our lint rules does not allow that, I think.

[janmiderback]

I think I agree 🙂

[peol]

LGTM

[janmiderback]

LGTM! Only one comment

[janmiderback]

nit - I know this is a matter of personal taste but I don't like early exits like this unless it is failure conditions.
I think is better to invert the logic and have if (this.running) { and then the code we want to run, avoiding multiple exit points if possible. Do as you please 🙂