Remove thread-unsafe runtime requires #43

brasic · 2023-06-28T14:58:41Z

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

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.

kares · 2023-06-29T12:58:01Z

lib/http/cookie.rb

@@ -4,10 +4,7 @@
 require 'uri'
 require 'domain_name'
 require 'http/cookie/ruby_compat'
-
-module HTTP
-  autoload :CookieJar, 'http/cookie_jar'


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

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)

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'

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.

kares · 2023-06-29T12:59:34Z

lib/http/cookie_jar.rb

@@ -342,3 +322,6 @@ def cleanup(session = false)
    self
  end
 end
+
+require 'http/cookie_jar/abstract_store'
+require 'http/cookie_jar/abstract_saver'


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

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

knu

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 · 2023-06-29T18:54:21Z

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!

<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'

Per request here: sparklemotion#43 (review)

brasic · 2023-07-03T21:38:42Z

This should be ready to review again.

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

brasic · 2023-07-03T21:42:16Z

lib/http/cookie_jar/yaml_saver.rb

-    def load_yaml(yaml)
-      YAML.load(yaml)
-    end
+  def load_yaml(yaml)


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.

kares · 2023-10-09T11:43:05Z

lib/http/cookie_jar/abstract_store.rb

+if defined?(JRUBY_VERSION)
+  class HTTP::CookieJar::MozillaStore
+    def initialize(*)
+      raise ArgumentError, "MozillaStore is not supported on JRuby"


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)

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

brasic requested a review from knu as a code owner June 28, 2023 14:58

brasic mentioned this pull request Jun 28, 2023

Thread-unsafe class caching #27

Open

brasic added 2 commits June 28, 2023 17:37

Add jruby runtime exclusion to abstract store

20d9927

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.

kares reviewed Jun 29, 2023

View reviewed changes

knu reviewed Jun 29, 2023

View reviewed changes

brasic added 3 commits June 30, 2023 09:48

Ensure YAML/Psych/SQLite3 are not loaded unless they are used

e8dcdb4

Per request here: sparklemotion#43 (review)

Skip loading sqlite3 entirely on jruby

5a71a8b

brasic commented Jul 3, 2023

View reviewed changes

brasic changed the title ~~Replace thread-unsafe runtime requires with vanilla autoload~~ Remove thread-unsafe runtime requires Jul 3, 2023

kares reviewed Oct 9, 2023

View reviewed changes

julien-pinchelimouroux mentioned this pull request Oct 26, 2023

Fix ArgumentError. #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove thread-unsafe runtime requires #43

Remove thread-unsafe runtime requires #43

brasic commented Jun 28, 2023 •

edited

kares Jun 29, 2023

brasic Jun 29, 2023

kares Jun 29, 2023

brasic Jun 29, 2023 •

edited

kares Jun 29, 2023

brasic Jun 29, 2023

knu left a comment

brasic commented Jun 29, 2023

brasic commented Jul 3, 2023 •

edited

brasic Jul 3, 2023

kares Oct 9, 2023

knu Nov 1, 2023

Remove thread-unsafe runtime requires #43

Are you sure you want to change the base?

Remove thread-unsafe runtime requires #43

Conversation

brasic commented Jun 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasic Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knu left a comment

Choose a reason for hiding this comment

brasic commented Jun 29, 2023

brasic commented Jul 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasic commented Jun 28, 2023 •

edited

brasic Jun 29, 2023 •

edited

brasic commented Jul 3, 2023 •

edited