Terminate client renewal goroutine if creating new session

[szli]

In Client's updateSession funciton, if renew time passes, we will call ASExchange and it will create a new session. However, the goroutine ran by enableAutoSessionRenewal is still running, when we create a new session we will start another goroutine to call updateSession. Hence, two goroutines will call updateSession. UpdateSession will create more sessions by calling ASExchange, which create new goroutines again. The renewal goroutine grows exponentially, which can cause serious problem. If we call ASExchange to add a new session, the previous goroutine should be terminated.

[jcmturner]

Thanks for raising this issue. I'll take a look into it.

[szli]

Looks like the way wait time is computed is also a problem.
w := (s.EndTime.Sub(time.Now().UTC()) * 5) / 6
This is not 1 min, this is 5/6 of the duration from now to EndTime.
If in updateSession, renew didn't happen, the s.EndTime does not change. We will keep looping with smaller and smaller w before it passes the end time. Looping will cause new goroutine being created, if we always do ASExchange in updateSession function.
Maybe simply have
w = s.EndTime.Sub(time.Now()) - 60 * Second
can solve the problem. This way, updateSession will be called only once if no renewal happens, then w<0, and goroutine returns. If renewal happens, EndTime will be updated, and we will continue to run this goroutine. The edge case is, if renewal happens, but endtime-now < 60s, w < 0, we will return without having a renewal goroutine running...

[szli]

I think I understand the reason for *5/6, it is for automatically retry if renewTGT fails. However, this causes problem if updateSession runs ASExchange(). In my case, RenewTill is zero, so it always creates new session. Why RenewTill is zero remains puzzle to me, because in command line, klist gives me a valid RenewTill time.

[jcmturner]

Branch "issue-132" has some updates that I think should resolve this. Are you able to try it and check if it resolves the issue for you?

Thanks.

[szli]

I think it solves the problem. Will this be merged to master soon?

[jcmturner]

Yes, I'll be looking to merge today and create a new release.

[jcmturner]

I was looking into merging this and realised the fix I had would not cover the case of code calling Login multiple times themselves so I have updated to give the auto renew goroutine a cancellation channel. Any time AddSession() is called any pre-exiting sessions have the cancellation signal sent so auto renew goroutines should exit.

Can you take another look and see if this fixes things for you. If so, I will get it merged to master.

It would be useful if you could share some details about how you tested and discovered this as I've been thinking about how to automate some tests around it and I'm not sure of the best way to do so. Any information you have may be useful to me. Thanks!

[szli]

I left a review comment.
We observed the extreme resource usage in a long running process, and took a while to find out it came from this library. To reproduce this error, I manually change sleep time to 10 sec, and observed the large amount of goroutines are created. In my opinion, these kind of functionality is hard to unittest because it has external dependency (krb servers)

[jcmturner]

Just doing some integration tests and then I'll merge. I have docker images for KDCs to test against.

[szli]

The change looks good to me. Thank you!