-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memory leaks when using Listener#stop #476
Comments
@ioquatix If you have a free moment, would you mind giving this proposal a look over? 🙏 There are some Rails PRs I would like to make which would depend on this issue's resolution. |
Do you have time to have a chat and go through the issue with me tomorrow? |
If you can make a specific PR, I can review it and provide feedback. Honestly, I read your issue description and I think oh no... that seems like a complicated design we are trying to fix. |
The `@listeners` variable is global state that holds a reference to each `Listener` instance and can only be cleared by calling `Listen.stop`. This prevents individual listeners from being garbage collected if they are abandoned before `Listen.stop` is called. This commit replaces `@listeners` with a comparable call to `ObjectSpace.each_object`. Partially addresses guard#476.
I'm not sure if you meant the original code seems complicated, or my proposals seem complicated? I implemented the first proposal (replacing |
I guess the problem here is global state.
I think the solution is just to get rid of the global state. Is it possible? |
I don't know if I can think of a way to iterate over all It's worth pointing out that Listen already appears to be broken on JRuby. But if JRuby compatibility is a future goal, I could perhaps bring back the |
@headius if you have a moment do you think you can take a look and comment on how we can make this work better and/or support JRuby? |
The The specs also pass for me locally on Darwin. I suspect that the specs that fail on Travis are launching a subprocess, since locally they run much slower than previous specs. Perhaps there's a timeout in place that's not giving the JRuby subprocess enough time? |
The `@listeners` variable holds a reference to each `Listener` instance created by `Listen.to` and can only be cleared by calling `Listen.stop`. This can prevent `Listener` instances from being garbage collected if they are abandoned before `Listen.stop` is called. This commit wraps `Listener` instances in `WeakRef` before adding them to `@listeners`, allowing them to be garbage collected. Partially addresses guard#476.
The `@listeners` variable holds a reference to each `Listener` instance created by `Listen.to` and can only be cleared by calling `Listen.stop`. This can prevent `Listener` instances from being garbage collected if they are abandoned before `Listen.stop` is called. This commit wraps `Listener` instances in `WeakRef` before adding them to `@listeners`, to allow them to be garbage collected. Partially addresses guard#476.
`Listen::Internals::ThreadPool` manages all listener threads. Thus the only way to kill and subsequently garbage collect listener threads is by calling the `Listen::Internals::ThreadPool.stop` method (typically done via `Listen.stop`). This is a problem when individual listeners must be stopped and abandoned for garbage collection. This commit removes `Listen::Internals::ThreadPool` in favor of listener instances managing their own threads. Partially addresses guard#476.
`Listen::Internals::ThreadPool` manages all listener threads. Thus the only way to kill and subsequently garbage collect listener threads is by calling the `Listen::Internals::ThreadPool.stop` method (typically done via `Listen.stop`). This is a problem when individual listeners must be stopped and abandoned for garbage collection. This commit removes `Listen::Internals::ThreadPool` in favor of listener instances managing their own threads. Partially addresses guard#476.
`Listen::Internals::ThreadPool` manages all listener threads. Thus the only way to kill and subsequently garbage collect listener threads is by calling the `Listen::Internals::ThreadPool.stop` method (typically done via `Listen.stop`). This is a problem when individual listeners must be stopped and abandoned for garbage collection. This commit removes `Listen::Internals::ThreadPool` in favor of listener instances managing their own threads. Partially addresses guard#476.
The `@listeners` variable holds a reference to each `Listener` instance created by `Listen.to` and can only be cleared by calling `Listen.stop`. This can prevent `Listener` instances from being garbage collected if they are abandoned before `Listen.stop` is called. This commit wraps `Listener` instances in `WeakRef` before adding them to `@listeners`, to allow them to be garbage collected. Partially addresses #476.
`Listen::Internals::ThreadPool` manages all listener threads. Thus the only way to kill and subsequently garbage collect listener threads is by calling the `Listen::Internals::ThreadPool.stop` method (typically done via `Listen.stop`). This is a problem when individual listeners must be stopped and abandoned for garbage collection. This commit removes `Listen::Internals::ThreadPool` in favor of listener instances managing their own threads. Partially addresses #476.
`Listen::Internals::ThreadPool` manages all listener threads. Thus the only way to kill and subsequently garbage collect listener threads is by calling the `Listen::Internals::ThreadPool.stop` method (typically done via `Listen.stop`). This is a problem when individual listeners must be stopped and abandoned for garbage collection. This commit removes `Listen::Internals::ThreadPool` in favor of listener instances managing their own threads. Partially addresses #476.
I've merged all the changes. Can you please check it? |
Yes, thank you @ioquatix! I wrote a benchmark script to show the difference: require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "benchmark-memory"
gem "listen", path: "."
end
require "benchmark/memory"
require "listen"
Benchmark.memory do |x|
x.report("warmup") do
Listen.to("lib") { }.start
Listen.stop
end
x.report("Listener#stop") do
Listen.to("lib") { }.tap(&:start).stop
end
end Running in the Before (at ba5059c):
or
depending on factors I can't seem to identify. After (at f72d44b):
|
Version 3.3 fixes memory leaks that occur when stopping individual listeners (see guard/listen#476).
I've noticed at least two memory "leaks" when using on
Listener#stop
(notListen.stop
).The first leak is the
@listeners
array in theListen
module. It is appended to when creating aListener
, but only cleared inListen.stop
(notListener#stop
). I suppose a workaround would be to useListener.new
directly instead ofListen.to
, but that doesn't seem to be the recommended usage (going by the README). Also note that this leak isn't merely ofListener
instances, but of potentially large object graphs that eachListener
instance's block holds a reference to.The second leak is
Listen::Internals::ThreadPool
. It is appended to inListen::Adapter::Base#start
andListen::Event::Loop#setup
, but only cleared inListen.stop
(notListener#stop
).My proposal to fix the first leak is to remove
@listeners
and replace its usage inListen.stop
withObjectSpace.each_object
. It will be slower than iterating an array, but it shouldn't be done often, so that shouldn't be an issue.My proposal to fix the second leak is to remove
Listen::Internals::ThreadPool
entirely, and store thread references directly in eachListen::Adapter::Base
andListen::Event::Loop
instance. ThenListen::Adapter::Base#stop
andListen::Event::Loop#teardown
can kill and clear an instance's own threads.I am completely new to the Listen code base, so please tell me if I am misunderstanding the situation, or if my ideas are laughably naive. 😅 However, if these proposals sound good, I am willing to submit a PR.
The text was updated successfully, but these errors were encountered: