provide a way to stop refresh goroutine
the refresh goroutine leaks forever and there's no way to stop it: https://github.com/KimMachineGun/automemlimit/blob/a9a712bee9977065cf72b4f29fcaf3a4e0573e13/memlimit/memlimit.go#L213
Thank you for the request @kruskall .
When would you want the refresh logic to stop? Do you have any specific points?
The current design assumes the refresh should run for the application's lifetime, so I simplified the interface.
Hi 👋
When would you want the refresh logic to stop? Do you have any specific points? The current design assumes the refresh should run for the application's lifetime, so I simplified the interface.
This is ok but some application might reload without ever stopping (e.g. logger might change). In automaxprocs this is easy because the Set method doesn't spawn any goroutine so it could be recreated/called but automemlimit is creating a refresh loop in the background each time.
https://github.com/KimMachineGun/automemlimit/blob/a9a712bee9977065cf72b4f29fcaf3a4e0573e13/memlimit/memlimit.go#L108
IMO even a new option to prevent starting the refresh process in the background would be a backward-compatible way of doing this
@kruskall
I thought there were two improvements I could make:
- Don't spawn a refresh goroutine when refresh is disabled. (It'll stop right after the goroutine is created, but it doesn't need to be spawned in the first place.)
- Provide a way to stop the refresh goroutine.
The first one was my mistake, so I released the fixed version as v0.7.3.
But the second one isn't included in this release. I don't want to make an API change since I'm currently working on v1.0.0. The second issue will definitely be handled in v1.0.0 with context.Context.
Until v1.0.0, please use custom refreshing logic (like what apm-server does for automaxprocs).
+ I'll leave this issue open till v1.0.0 is released.