Client does not get cleaned up properly

[Rukenshia]

Hi there,

I have built an application using this library - it works really well, so thanks for creating it! I have a background goroutine that tries to sync some data between Trello and another service every couple of minutes. In my loop, I create a new trello.Client via NewClient and noticed that over time, my CPU usage becomes really high. I think it might be related to the throttling built into the library, because time.Tick does not get cleaned up properly (apparently you're supposed to use time.NewTicker and stop it, ref).
I'm not 100% sure that this is the exact cause, but if I do not create a trello client in my loop, the CPU usage stays the same over time.

I'm not sure if there is some sort of best practice on how to resolve this so I wanted to open an issue first - maybe there can be an option to disable the throttling or expose the ticker so that I can stop it myself when I know I won't need the Client anymore? Happy to help out with the solution.

Cheers and thanks again for the library!

Example code (might not work, I removed some parts of my application)

		for {
			time.Sleep(5 * time.Minute)

			for _, user := range s.Users {
				trelloClient := trello.NewClient(user.TrelloAppKey, user.TrelloToken)

				// ... do things with the client
			}
		}

[adlio]

I'm not 100% sure that this is the exact cause, but if I do not create a trello client in my loop, the CPU usage stays the same over time.

Seems like enough of a reason to investigate for me. I'll leave this issue open and investigate as soon as I get some time.

[Rukenshia]

I did some testing this morning @adlio and it seems like using the rate package might help us out here. I opened a new PR :)

[TJM]

First suggestion that just jumped out at me would be to have your "User" object have a trelloClient *trello.Client and not create a new client every 5 minutes. I would also mention that unless the size of s.Users is quite small, or you are not sync'ing much, you are going to have some issues completing your loop in a reasonable time. I think Trello actually specifically recommends against polling loops and thats why they have Webhooks. https://blog.trello.com/webhooks-are-here :)

Anyhow, I am not saying that the issue you reported isn't an issue or rate.Limiter isn't a good fix, but maybe my suggestions will help overall? :)

~tommy

[Rukenshia]

First suggestion that just jumped out at me would be to have your "User" object have a trelloClient *trello.Client and not create a new client every 5 minutes. I would also mention that unless the size of s.Users is quite small, or you are not sync'ing much, you are going to have some issues completing your loop in a reasonable time. I think Trello actually specifically recommends against polling loops and thats why they have Webhooks. https://blog.trello.com/webhooks-are-here :)

Anyhow, I am not saying that the issue you reported isn't an issue or rate.Limiter isn't a good fix, but maybe my suggestions will help overall? :)

~tommy

I already did that - right after finding out that it is an issue. I was creating the client whenever I needed it because I thought it was disposable and I haven't encountered another package that was having issues with CPU usage whenever a new Client is created.
I changed the User object to call a method that creates a client if there isn't any or their settings changed :)

I'm not using webhooks as the application is not running on a server with a public ip address, but I plan on changing that.

Thanks for your hints though!

[TJM]

Yea, I was going to suggest having a getTrelloClient() method that did some sort of checksum or something, good call.

All too familiar with the internal server thing. :)