Skip to content

Fix memory leak in PerThreadPersistentClients #57

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

Closed
wants to merge 1 commit into from

Conversation

Boberkraft
Copy link

@Boberkraft Boberkraft commented Aug 6, 2025

Hello. I noticed two memory leaks:

  1. To set clients, we were using thread.thread_variable_set(@key), but we read them via thread[@key]. Since one is Thread-local and the other Fiber-local, we were never finding any clients to close
  2. The thread variable symbol @key was leaking (https://redmine.ruby-lang.org/issues/19333)

To fix both, i used this pattern that you have used previously used socketry/async-http@fcf231a#diff-df1daaada169fc5385e7257d57137dc5e11165a5551dda1ccc17f7546f1c360bR8

Types of Changes

  • Bug fix.

Contribution

@Boberkraft
Copy link
Author

Actually i misunderstood what this code actually does, so my fix is wrong, as it will close all clients

@Boberkraft Boberkraft closed this Aug 6, 2025
@ioquatix
Copy link
Member

ioquatix commented Aug 6, 2025

Thanks, actually something doesn't look quite right in this code. Let me review it.

@Boberkraft
Copy link
Author

Boberkraft commented Aug 6, 2025

I wanted also to fix the deleting of thread variable, since it's leaky:

    179: def close
    180: 	Thread.list.each do |thread|
 => 181: 		binding.pry
    182: 
    183: 		if clients = thread.thread_variable_get(@key)
    184: 			thread.thread_variable_set(@key, nil)
    185: 
    186: 			clients.close
    187: 		end
    188: 	end
    189: end

3.4.3 :001 > thread.thread_variable_get(@key)
=> #<Async::HTTP::Faraday::PersistentClients:0x000000012d614828
 ...>
3.4.3 :001 > thread.thread_variable_set(@key, nil)
=> nil
3.4.3 :001 > thread.thread_variables
=> [:sentry_hub, :"Async::HTTP::Faraday::PerThreadPersistentClients_15176"]

But i don't think it's possible without adding something like a hash as a thread variables that will hold the clients

@ioquatix
Copy link
Member

ioquatix commented Aug 7, 2025

This has been discussed: https://bugs.ruby-lang.org/issues/19333

IIRC, Matz accepted this change so maybe we could implement it. I was probably too conservative not to implement it... it might break stuff. But I think that this use case is valid so we should fix it.

Do you want to make a PR to CRuby?

@Boberkraft
Copy link
Author

I might try on weekend. I have never touched CRuby. We will see :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants