search

zombocom / puma_worker_killer

Automatically restart Puma cluster workers based on max RAM available
750 stars 77 forks source link

Dont kill PumaWorkerKiller when calling largest_worker.term a second time #9

Closed Leooo closed 5 years ago

Leooo commented 9 years ago

Hello, I'm trying to use PumaWorkerKiller together with Rails ActionController::Live streaming.

Puma seems to have a problem restarting a worker if some SSEs connections are opened (even if the connections are on other workers see https://github.com/puma/puma/issues/569#issuecomment-68013757).

In any case, there may be some delay between the SIGTERM being sent to a worker by PumaWorkerKiller and the effective restart of the worker (if only to give time for the SSEs connections to close).

It seems PumaWorkerKiller gets killed (exception?) when sending a second time largest_worker.term in PumaWorkerKiller:: PumaMemory. I can get around the issue by testing if a restart order has already happened before largest_worker.term evaluates, and not sending the order again if that's the case.

schneems commented 9 years ago

Any ideas on how to fix this problem? How does unicorn worker killer deal with this issue? Are there docs we can add?

Leooo commented 9 years ago

Never used Unicorn (only Thin), and I'm quite a newbie developer, but on my side I hacked your gem in vendor and added a flag to my database when largest_worker.term is run, and a boolean to stop it running twice when the flag is true (flag is then set to false on on_worker_boot). This is very dirty.

schneems commented 9 years ago

Interesting, I could switch over to have some kind of over-writeable block or callback or plugin system similar to how puma auto tune works

carljohnstone commented 8 years ago

Hi, I've had practically the same problem - I've traced it back to @options being undefined here - https://github.com/puma/puma/blob/master/lib/puma/cluster.rb#L86 - in older versions of puma, as long as you upgrade puma past v2.11.0 it's good.

schneems commented 8 years ago

Maybe we should bump the minimum version of puma in the gemspec?