Fix memory leak in PostHog::Client.new #43
Conversation
The PostHog::Client calls Kernel#at_exit at each instantiation, which registers a process-global callback for each Client instance. This global callback is represented by a T_FILE entry in ObjectSpace.count_objects and is never GC'd for the life of the process.
Any process that instantiates multiple clients will be susceptible to this memory leak, which is very common in almost any long-running process, but in particular, the norm for Sidekiq / Resque environments.
The good news is this line is completely unnecessary, since in the current implementation, the worker thread does not block the process from exiting, which means that setting this thread variable is unlikely to actually cause the thread to "gracefully exit" unless the code also blocked on Thread execution via Thread#join. In other words, because Thread#join is not used, this line is a no-op. This is all mostly irrelevant anyway, since there is no actual cleanup that is not already being performed by the basic exiting of a process, and an at_exit callback (or graceful exit logic) is simply not needed for HTTP clients.
To reproduce the leak:
```ruby
GC.disable
p ObjectSpace.count_objects[:T_FILE]
10.times { PostHog::Client.new; GC.start }
p ObjectSpace.count_objects[:T_FILE]
```
|
Thanks @lsegal! Getting someone to review this shortly 👍 |
|
Will publish a new version shortly 👍 |
Curious about this point - for e.g. a web server process, is it expected to initialize one client and share it rather than instantiating one in each request that needs to communicate with PostHog? It does seem like instantiating a PostHog client is a bit expensive so it might make sense to do it once at server initialization time... though I dunno if you'd have to do it in a thread-safe way and if there's any state on a PostHog client that would be undesirable to persist across requests. |
|
Nothing is stopping you from instantiating a single object if you plan on using this from your webapp server itself. I can't personally speak to how qualified the "a bit expensive" statement is, but if it is expensive, you can indeed share the instance, it already was thread-safe before this patch, and this patch doesn't change anything about it's thread safety. |
|
Yeah that's fair, with the memory leak fixed the impact of making a new client is less severe, and it might already not be noticeable - I just noticed it because I'm working on a newish app and was profiling memory usage. That's good to know that it was thread safe already! I was just curious because you specifically called out queue worker processes but I guess web servers also qualify as long lived. |
|
Hmm, did a little more digging into this because I happened to notice that there was nothing that makes a thread set posthog-ruby/lib/posthog/send_worker.rb Line 37 in 7a92241 It looks like in analytics-ruby, the gem on which this was based, this was actually added to avoid JRuby memory leaks. Kinda funny that it ended up causing a memory leak in other usage patterns. For segment they do recommend initializing it as a constant in a Rails initializer: https://segment.com/docs/connections/sources/catalog/libraries/server/ruby/#getting-started. And it makes sense to me since the whole Queue/Thread pattern seems designed for long-running clients rather than spinning up a client on each request. In some somewhat anecdotal testing (cURLing the homepage of my app repeatedly), it feels like my application is a few ms faster on average when I initialize it in an initializer rather than on each request, which makes sense since it's doing less work per request. So I think this is the way to go for a Rails app! And maybe for a background worker too if it can be initialized once and shared. |
The PostHog::Client calls Kernel#at_exit at each instantiation, which registers a process-global callback for each Client instance. This global callback is represented by a T_FILE entry in ObjectSpace.count_objects and is never GC'd for the life of the process.
Any process that instantiates multiple clients will be susceptible to this memory leak, which is very common in almost any long-running process, but in particular, the norm for Sidekiq / Resque environments.
The good news is this line is completely unnecessary, since in the current implementation, the worker thread does not block the process from exiting, which means that setting this thread variable is unlikely to actually cause the thread to "gracefully exit" unless the code also blocked on Thread execution via Thread#join. In other words, because Thread#join is not used, this line is a no-op. This is all mostly irrelevant anyway, since there is no actual cleanup that is not already being performed by the basic exiting of a process, and an at_exit callback (or graceful exit logic) is simply not needed for HTTP clients.
To reproduce the leak:
This fix will likely improve use cases for users in #10.