Make XmlMini.with_backend usable with threads #8219

Merged
merged 1 commit into from Nov 15, 2012

Conversation

Projects
None yet
3 participants
@nikitug
Contributor

nikitug commented Nov 14, 2012

XmlMini.with_backend now may be safely used with threads:

Thread.new do
  XmlMini.with_backend("REXML") { rexml_power }
end
Thread.new do
  XmlMini.with_backend("LibXML") { libxml_power }
end

Each thread will use it's own backend.

@rafaelfranca

View changes

activesupport/lib/active_support/xml_mini.rb
+ end
+
+ def cast_backend_name_to_module(name)
+ return name unless name

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 14, 2012

Member

Why this check?

@rafaelfranca

rafaelfranca Nov 14, 2012

Member

Why this check?

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 14, 2012

Contributor

@rafaelfranca to be sure we cast nil properly.

if name.nil?
  nil
elsif name.is_a?(Module)    
  name
else

Would be more readable here, wdyt?

Or Thread.current[:xml_mini_backend] = name && cast_backend_name_to_module(name) is better?

@nikitug

nikitug Nov 14, 2012

Contributor

@rafaelfranca to be sure we cast nil properly.

if name.nil?
  nil
elsif name.is_a?(Module)    
  name
else

Would be more readable here, wdyt?

Or Thread.current[:xml_mini_backend] = name && cast_backend_name_to_module(name) is better?

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 14, 2012

Member

And what are the case where name is nil? Maybe we can avoid nil in the source, unless we want to allow the users to pass nil

@rafaelfranca

rafaelfranca Nov 14, 2012

Member

And what are the case where name is nil? Maybe we can avoid nil in the source, unless we want to allow the users to pass nil

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 14, 2012

Contributor

This call will try to cast nil: self.current_thread_backend = nil, but it's not a public API, so it should be ok to use name && cast_backend_name_to_module(name).

@nikitug

nikitug Nov 14, 2012

Contributor

This call will try to cast nil: self.current_thread_backend = nil, but it's not a public API, so it should be ok to use name && cast_backend_name_to_module(name).

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 14, 2012

Member

Yes, seems fine

@rafaelfranca

rafaelfranca Nov 14, 2012

Member

Yes, seems fine

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 14, 2012

Contributor

@rafaelfranca done, thanks for review!

@nikitug

nikitug Nov 14, 2012

Contributor

@rafaelfranca done, thanks for review!

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 14, 2012

Member

I'm getting errors in the tests.

/Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require': cannot load such file -- libxml (LoadError)
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/xml_mini/libxml.rb:1:in `<top (required)>'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/test/xml_mini_test.rb:3:in `<top (required)>'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:9:in `each'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:9:in `block in <main>'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:4:in `select'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:4:in `<main>'

Also it needs a rebase

Member

rafaelfranca commented Nov 14, 2012

I'm getting errors in the tests.

/Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require': cannot load such file -- libxml (LoadError)
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/xml_mini/libxml.rb:1:in `<top (required)>'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/test/xml_mini_test.rb:3:in `<top (required)>'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `block in require'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:212:in `load_dependency'
    from /Users/rafaelmfranca/Projects/github/rails/activesupport/lib/active_support/dependencies.rb:227:in `require'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:9:in `each'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:9:in `block in <main>'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:4:in `select'
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3-p327/lib/ruby/gems/1.9.1/gems/rake-10.0.1/lib/rake/rake_test_loader.rb:4:in `<main>'

Also it needs a rebase

@nikitug

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 15, 2012

Contributor

@rafaelfranca oops, my bad. Seems like I've push -f some old code. That's two-workspaces-specific problem 😃 Fixed.

Contributor

nikitug commented Nov 15, 2012

@rafaelfranca oops, my bad. Seems like I've push -f some old code. That's two-workspaces-specific problem 😃 Fixed.

+ assert_equal REXML, @xml.backend
+ end
+ assert_equal REXML, @xml.backend
+ end

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 15, 2012

Member

Doesn't this belong to the other WithBackendTest?

@carlosantoniodasilva

carlosantoniodasilva Nov 15, 2012

Member

Doesn't this belong to the other WithBackendTest?

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 15, 2012

Contributor

@carlosantoniodasilva you're right, thanks! 😃 Fixed.

@nikitug

nikitug Nov 15, 2012

Contributor

@carlosantoniodasilva you're right, thanks! 😃 Fixed.

@rafaelfranca

View changes

activesupport/lib/active_support/xml_mini.rb
- @backend = ActiveSupport.const_get("XmlMini_#{name}")
- end
+ self.current_thread_backend = name if current_thread_backend
+ @backend = name && cast_backend_name_to_module(name)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 15, 2012

Member

I was talking with @carlosantoniodasilva and we think is better to store the casted model in a local variable and pass it to the thread local and to the instance variable. This would avoid cast the name twice. Something like this:

backend = name && cast_backend_name_to_module(name)
self.current_thread_backend = backend if current_thread_backend
@backend = backend
@rafaelfranca

rafaelfranca Nov 15, 2012

Member

I was talking with @carlosantoniodasilva and we think is better to store the casted model in a local variable and pass it to the thread local and to the instance variable. This would avoid cast the name twice. Something like this:

backend = name && cast_backend_name_to_module(name)
self.current_thread_backend = backend if current_thread_backend
@backend = backend

This comment has been minimized.

Show comment Hide comment
@nikitug

nikitug Nov 15, 2012

Contributor

@rafaelfranca yep, makes sense! Done

@nikitug

nikitug Nov 15, 2012

Contributor

@rafaelfranca yep, makes sense! Done

Make XmlMini.with_backend usable with threads
`XmlMini.with_backend` now may be safely used with threads:

  Thread.new do
    XmlMini.with_backend("REXML") { rexml_power }
  end
  Thread.new do
    XmlMini.with_backend("LibXML") { libxml_power }
  end

Each thread will use it's own backend.

rafaelfranca added a commit that referenced this pull request Nov 15, 2012

Merge pull request #8219 from nikitug/threadsafe_xmlmini_with_backend
Make XmlMini.with_backend usable with threads

Conflicts:
	activesupport/CHANGELOG.md
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 15, 2012

Member

Thank you

Member

rafaelfranca commented Nov 15, 2012

Thank you

@rafaelfranca rafaelfranca merged commit 1fab200 into rails:master Nov 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment