Skip to content
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

Remove thread-unsafe runtime requires #43

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

brasic
Copy link

@brasic brasic commented Jun 28, 2023

Dynamically requiring implementations at runtime in this way is not safe in a multithreaded program, even in MRI with the GIL. We can simplify this while retaining identical performance by just using autoload for the AbstractStore combined with plain requires, turning the @@class_map into a simple case statement.

(edit: I tested this more thoroughly under several JRuby and TruffleRuby versions and my original fix did not fully solve the issue, the autoloads were still racy. So I pushed a subsequent commit to just require all implementations up front)

I'm not sure dynamic selection of the implementation is even really necessary but I left it in to avoid changing too much in one PR.

Fixes #27
Fixes #6

Dynamically requiring implementations at runtime in this way is not safe
in a multithreaded program, even in MRI with the GIL. We can simplify
this by just using autoload and turning the @@class_map into a simple
case statement.

Fixes sparklemotion#27
@brasic brasic requested a review from knu as a code owner June 28, 2023 14:58
brasic added 2 commits June 28, 2023 17:37
Repro:

    require 'http/cookie'

    # We only care about the first exception, not all of them
    Thread.report_on_exception = false

    100.times.map do
      Thread.new do
        HTTP::CookieJar::HashStore.new
      end
    end.each(&:join)

The above will reliably trigger an exception on all versions of
TruffleRuby and many versions of JRuby.  For example:

        $ ruby -v
        truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
        $ ruby repro.rb
        /home/cbrasic/repos/http-cookie/lib/http/cookie_jar/abstract_store.rb:50:in `initialize': undefined method `each_pair' for nil:NilClass (NoMethodError)
                from repro.rb:10:in `block (2 levels) in <main>'

        $ jruby --version
        jruby 9.1.2.0 (2.3.0) 2016-05-26 7357c8f OpenJDK 64-Bit Server VM 25.372-b07 on 1.8.0_372-b07 +jit [linux-x86_64]
        $ jruby repro.rb
        NameError: can't resolve constant HashStore after loading http/cookie_jar/hash_store
              const_missing at /home/cbrasic/.asdf/installs/ruby/jruby-9.1.2.0/lib/ruby/gems/shared/gems/http-cookie-1.0.5/lib/http/cookie_jar.rb:25
          block in repro.rb at repro.rb:10
gemspec excludes sqlite3 from installing under jruby, so the
mozilla_store.rb file will raise LoadError under JRuby.

To avoid an undefined constant under jruby define a fake one that just
raises.
@@ -4,10 +4,7 @@
require 'uri'
require 'domain_name'
require 'http/cookie/ruby_compat'

module HTTP
autoload :CookieJar, 'http/cookie_jar'
Copy link

Choose a reason for hiding this comment

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

autoload is fine - there should be no need to change this

Copy link
Author

Choose a reason for hiding this comment

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

I thought so too, but I tested the first commit (which left autoload in) on all truffleruby versions and older jrubies and it still exhibited thread unsafety. Here's a repro:


   require 'http/cookie'

    # We only care about the first exception, not all of them
    Thread.report_on_exception = false

    100.times.map do
      Thread.new do
        HTTP::CookieJar::HashStore.new
      end
    end.each(&:join)

Copy link

Choose a reason for hiding this comment

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

that would be related to other changes I do not see this reproducing in 9.2/9.3/9.4,
what I did on top of the PR:

diff --git lib/http/cookie.rb lib/http/cookie.rb
index 278036a..3e8f421 100644
--- lib/http/cookie.rb
+++ lib/http/cookie.rb
@@ -4,7 +4,10 @@ require 'time'
 require 'uri'
 require 'domain_name'
 require 'http/cookie/ruby_compat'
-require 'http/cookie_jar'
+#require 'http/cookie_jar'
+module HTTP
+  autoload :CookieJar, 'http/cookie_jar'
+end
 
 # This class is used to represent an HTTP Cookie.
 class HTTP::Cookie

again I am fine with the require here given there isn't a lot going on
but maybe the autoload should be kept (due compatibility) at the nested level for these:

require 'http/cookie_jar/abstract_store'
require 'http/cookie_jar/abstract_saver'

Copy link
Author

@brasic brasic Jun 29, 2023

Choose a reason for hiding this comment

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

I took another look and found that I had a bug in the initial implementation. 2d5b65a (on another branch for now) is a commit on top of bf6391b that should work properly by making all six classes under HTTP::CookieJar autoloaded. I thought that addressed everything but in my testing truffleruby still has a (much rarer) issue with it. I added a task to verify safety automatically in 99dd9e4 and I get inconsistent results for truffle sometimes with the autoload approach.

~/repos/http-cookie $ rake thread_safety_check
checking for thread safety bug with 1000 threads on truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
no issues detected
~/repos/http-cookie $ rake thread_safety_check
checking for thread safety bug with 1000 threads on truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
no issues detected
~/repos/http-cookie $ rake thread_safety_check
checking for thread safety bug with 1000 threads on truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
no issues detected
~/repos/http-cookie $ rake thread_safety_check
checking for thread safety bug with 1000 threads on truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
1 of 1000 threads saw exceptions: ["NameError"]
rake aborted!
NameError: uninitialized constant HTTP::CookieJar::HashStore
/home/cbrasic/repos/http-cookie/Rakefile:34:in `const_missing'
/home/cbrasic/repos/http-cookie/Rakefile:34:in `block (3 levels) in <top (required)>'
Tasks: TOP => thread_safety_check
(See full trace by running task with --trace)

OTOH 2d5b65a does fix this for all jrubies I've tried. I've been unable to reproduce the issue with MRI anywhere even though internally we have encountered this issue with it in real workloads.

@@ -342,3 +322,6 @@ def cleanup(session = false)
self
end
end

require 'http/cookie_jar/abstract_store'
require 'http/cookie_jar/abstract_saver'
Copy link

Choose a reason for hiding this comment

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

these could be autoload-ed esp. if require 'http/cookie_jar' is eager

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, although after trying that I am still trying to understand why this rake task sometimes indicates thread safety issues under recent versions of truffleruby. autoloading is atomic in ruby, but only with respect to the individual constant being autoloaded. I thought I removed any mutation of other classes during autoloading but I might have missed something. Will dig further.

lib/http/cookie_jar/abstract_store.rb Outdated Show resolved Hide resolved
Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this. It's okay for me to remove the problematic autoloading of http-cookie sub-modules, but I just don't want external libraries like psych/yaml and sqlite3 to be eager-loaded. Can you get them autoloaded?

@brasic
Copy link
Author

brasic commented Jun 29, 2023

I just don't want external libraries like psych/yaml and sqlite3 to be eager-loaded. Can you get them autoloaded?

@knu I'm happy to!

brasic added 3 commits June 30, 2023 09:48
    <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85: warning: <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85: warning: loading in progress, circular require considered harmful - /home/cbrasic/repos/http-cookie/lib/http/cookie_jar.rb
            from /home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/gems/3.3.0+0/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:6:in  `<main>'
            from /home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/gems/3.3.0+0/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:6:in  `select'
            from /home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/gems/3.3.0+0/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:21:in  `block in <main>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/test/test_http_cookie.rb:2:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/test/helper.rb:4:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/lib/http/cookie.rb:7:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar.rb:325:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar/abstract_store.rb:115:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar/hash_store.rb:2:in  `<top (required)>'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
            from <internal:/home/cbrasic/.asdf/installs/ruby/3.3.0-dev/lib/ruby/3.3.0+0/rubygems/core_ext/kernel_require.rb>:85:in  `require'
@brasic
Copy link
Author

brasic commented Jul 3, 2023

This should be ready to review again.

@knu, let me know if e8dcdb492a46c49049b30a4d399cdd3510e236e0 addresses your concern.

def load_yaml(yaml)
YAML.load(yaml)
end
def load_yaml(yaml)
Copy link
Author

Choose a reason for hiding this comment

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

I rewrote this method to avoid negating the requested introduction of autoload :YAML. It adds an extra level of indirection to every load, but only on extremely old and unsupported rubies.

@brasic brasic changed the title Replace thread-unsafe runtime requires with vanilla autoload Remove thread-unsafe runtime requires Jul 3, 2023
if defined?(JRUBY_VERSION)
class HTTP::CookieJar::MozillaStore
def initialize(*)
raise ArgumentError, "MozillaStore is not supported on JRuby"
Copy link

Choose a reason for hiding this comment

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

this isn't that of a nice "idiomatic" Ruby approach.
if a constant is not supported it should not be defined (and I would rather prefer HTTP::CookieJar::MozillaStore to raise a missing constant, instead of an ArgumentError on initialize)

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Change this to just unless defined?(JRUBY_VERSION) for now and expect someone to come up with an implementation for JRuby.

@sensorsasha
Copy link

Is there any chance this can get merged soon?

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.

Thread-unsafe class caching undefined method 'implementation' for HTTP::CookieJar::AbstractStore:Class
4 participants