Skip to content

Fix Thread leaking in ThreadLocalVariables #164

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.

Sign up for GitHub

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

Merged
jdantonio merged 4 commits into from
Nov 14, 2014
Merged

Fix Thread leaking in ThreadLocalVariables #164

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da60120
Fix Thread leaking in ThreadLocalVariables
pitr-ch Oct 5, 2014
7d33d7e
Fixing ThreadLocalVar to use 'ref' gem
pitr-ch Nov 8, 2014
cba0cd6
Added Ref gem dependency on non-JRuby systems.
jdantonio Nov 11, 2014
1159d5a
Removed Ref from the Gemfile.
jdantonio Nov 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ gemspec
group :development do
gem 'rake', '~> 10.3.2'
gem 'rake-compiler', '~> 0.9.2'
gem 'ref', '~> 1.0.5'
end

group :testing do
Expand Down
70 changes: 46 additions & 24 deletions lib/concurrent/atomic/thread_local_var.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,59 @@

module Concurrent

module ThreadLocalRubyStorage
class AbstractThreadLocalVar

def allocate_storage
@storage = Atomic.new Hash.new
end
module ThreadLocalRubyStorage

def get
@storage.get[Thread.current]
end
protected

def set(value)
@storage.update { |s| s.merge Thread.current => value }
end
begin
require 'ref'
rescue LoadError
raise LoadError,
Copy link
Member

@jdantonio jdantonio Nov 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of raising an error and refusing to work, could we instead output a warning and fall back to the original, degraded implementation? Leave it to the individual user to decide if they want to use the ref gem.

This is the approach the elasticsearch gem uses to enable persistent connections. It detects the presence of either typhoeus or patron and uses that gem when available. Without either of those optional dependencies the elasicesearch gem still works, it just doesn't perform as well.

Copy link
Member Author

@pitr-ch pitr-ch Nov 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me a memory leak means broken not just degraded implementation I would rather not provide it at all. As an user when I miss the warning about memory leak ending up debugging it for hours I would be much more frustrated that adding one dependency at the beginning. From my experience I really like libraries which are failing early explaining the problem. If you really want to avoid the dependency I could reimplement the weak_key_map for concurrent-ruby but than we'll have to maintain it.

Copy link
Member

@jdantonio jdantonio Nov 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a bug and not just degraded functionality then we need to fix it. I have mixed feelings about the Ref gem. For pragmatism and expedience I'll probably suggest that for now we use Ref, but let me explain in more detail my concerns about adding dependencies to gems such as our. Adding dependencies to utility gems can lead to four problems, all of which I have experienced in production systems:

  1. Downstream incompatibilities: Once gem depends on other gems which depend on other gems and so on. Adding one gem dependency results in a tree of several gem dependencies being added. Incompatibilities then occur downstream. (This is the exact problem Bundler was created to solve).
  2. Code bloat: A gem dependency is added for one class/function, but the dependency gem has thousands of lines of code (internally and in its dependencies). That one class/function unnecessarily bloats the code base.
  3. Compilation errors: I work in a cross-platform shop and we regularly have problems with C extension which don't compile. This is especially problematic when I require a pure-Ruby gem that has a downstream dependency which uses C extensions. I'be only recently started working with Java native extensions but I presume this can be a problem for JRuby, too. (This is why I pre-compile various builds of this gem.)
  4. Abandonment: A dependency is added but the maintainers stop maintaining it.

The Ref gem passes two of these four metrics. It doesn't have any downstream dependencies and it is a very lean gem, so it's good for items 1 and 2 above. Not so much for items 3 and 4. With respect to item 3, Ref includes Java native extensions but it does not pre-compile those extensions. Therefore we cannot guarantee that any particular user of ours will be able to successfully install our gem on JRuby. With respect to item 4, Ref has not been updated since May of 2013. There are six open issues dating back to April 2012 and the maintainer has only responded to one of them. There is a PR that has been open since August of 2013 and the maintainer has not responded. As far as I can tell Ref has been abandoned by its maintainer.

I appreciate that it would be a bunch of extra work for us to implement our own weak key map, but we need to maintain the integrity of our own gem.

So I'm OK if we want to include Ref for now, but the best long-term solution is probably to build our own weak key map. Perhaps we can make that a goal for 1.0.

I may also reach out to the gem author. If Ref is no longer being actively maintained we may have an opportunity to bring it into the ruby-concurrency organization.

Copy link
Member Author

@pitr-ch pitr-ch Nov 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a very good point the 4th one. I agree we should either reimplement or pull the gem under concurrent-ruby in long-term. Technically it passes 3 because on JRuby the gem is not being loaded so gem 'ref', platform: :mri will avoid any problems with compilation on JRuby, that should make the short-term solution even more viable.

Copy link
Member

@jdantonio jdantonio Nov 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about item 3. Even though we won't be using Ref in our JRuby gem, it will still be installed if we list the dependency in the gemspec. Compilation would fail on install even if we never load Ref at runtime. But now that I think it through, we can easily solve this. Since it isn't being used under JRuby I can added a guard in our gemspec to not require Ref when we create our pre-compiled JRuby build. Then Ref will never be installed.

I'll make the update on this branch later this evening.

'ThreadLocalVar requires ref gem installed on MRI to avoid memory leaks. It\'s not concurrent-ruby dependency.'
end

end
def allocate_storage
@storage = Ref::WeakKeyMap.new
end

module ThreadLocalJavaStorage
def get
@storage[Thread.current]
end

protected
def set(value, &block)
key = Thread.current

def allocate_storage
@var = java.lang.ThreadLocal.new
end
@storage[key] = value

def get
@var.get
if block_given?
begin
block.call
ensure
@storage.delete key
end
end
end
end

def set(value)
@var.set(value)
end
module ThreadLocalJavaStorage

end
protected

class AbstractThreadLocalVar
def allocate_storage
@var = java.lang.ThreadLocal.new
end

def get
@var.get
end

def set(value)
@var.set(value)
end

end

NIL_SENTINEL = Object.new

Expand All @@ -58,13 +76,17 @@ def value
end

def value=(value)
bind value
end

def bind(value, &block)
if value.nil?
stored_value = NIL_SENTINEL
else
stored_value = value
end

set stored_value
set stored_value, &block

value
end
Expand Down
1 change: 0 additions & 1 deletion lib/concurrent/atomics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,4 @@
require 'concurrent/atomic/cyclic_barrier'
require 'concurrent/atomic/count_down_latch'
require 'concurrent/atomic/event'
require 'concurrent/atomic/thread_local_var'
require 'concurrent/atomic/synchronization'
46 changes: 37 additions & 9 deletions spec/concurrent/atomic/thread_local_var_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@

module Concurrent

require 'concurrent/atomic/thread_local_var'

describe ThreadLocalVar do

subject{ ThreadLocalVar.new }
subject { ThreadLocalVar.new }

context '#initialize' do

Expand All @@ -20,7 +22,7 @@ module Concurrent
end

it 'sets the same initial value for all threads' do
v = ThreadLocalVar.new(14)
v = ThreadLocalVar.new(14)
t1 = Thread.new { v.value }
t2 = Thread.new { v.value }
expect(t1.value).to eq 14
Expand All @@ -29,11 +31,37 @@ module Concurrent

if jruby?
it 'uses ThreadLocalJavaStorage' do
expect(subject.class.ancestors).to include(Concurrent::ThreadLocalJavaStorage)
expect(subject.class.ancestors).to include(Concurrent::AbstractThreadLocalVar::ThreadLocalJavaStorage)
end
else
it 'uses ThreadLocalNewStorage' do
expect(subject.class.ancestors).to include(Concurrent::ThreadLocalRubyStorage)
expect(subject.class.ancestors).to include(Concurrent::AbstractThreadLocalVar::ThreadLocalRubyStorage)
end
end
end

unless jruby?
context 'GC' do
it 'does not leave values behind when bind is used' do
var = ThreadLocalVar.new(0)
100.times.map do |i|
Thread.new { var.bind(i) { var.value } }
end.each(&:join)
var.value = 0
expect(var.instance_variable_get(:@storage).keys.size).to be == 1
end

it 'does not leave values behind when bind is not used' do
var = ThreadLocalVar.new(0)
100.times.map do |i|
Thread.new { var.value = i; var.value }
end.each(&:join)
var.value = 0
sleep 0.1
GC.start
sleep 0.1
GC.start
expect(var.instance_variable_get(:@storage).keys.size).to be == 1
end
end
end
Expand All @@ -46,7 +74,7 @@ module Concurrent
end

it 'returns the value after modification' do
v = ThreadLocalVar.new(14)
v = ThreadLocalVar.new(14)
v.value = 2
expect(v.value).to eq 2
end
Expand All @@ -56,7 +84,7 @@ module Concurrent
context '#value=' do

it 'sets a new value' do
v = ThreadLocalVar.new(14)
v = ThreadLocalVar.new(14)
v.value = 2
expect(v.value).to eq 2
end
Expand All @@ -67,14 +95,14 @@ module Concurrent
end

it 'does not modify the initial value for other threads' do
v = ThreadLocalVar.new(14)
v = ThreadLocalVar.new(14)
v.value = 2
t = Thread.new { v.value }
t = Thread.new { v.value }
expect(t.value).to eq 14
end

it 'does not modify the value for other threads' do
v = ThreadLocalVar.new(14)
v = ThreadLocalVar.new(14)
v.value = 2

b1 = CountDownLatch.new(2)
Expand Down