automemlimit icon indicating copy to clipboard operation
automemlimit copied to clipboard

provide a way to stop refresh goroutine

Open kruskall opened this issue 8 months ago • 3 comments

the refresh goroutine leaks forever and there's no way to stop it: https://github.com/KimMachineGun/automemlimit/blob/a9a712bee9977065cf72b4f29fcaf3a4e0573e13/memlimit/memlimit.go#L213

kruskall avatar Jun 10 '25 13:06 kruskall

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.

KimMachineGun avatar Jun 11 '25 02:06 KimMachineGun

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 avatar Jun 12 '25 13:06 kruskall

@kruskall

I thought there were two improvements I could make:

  1. 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.)
  2. 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.

KimMachineGun avatar Jun 14 '25 14:06 KimMachineGun