From bf6391b51e612514149c6026d5ad080295aadd03 Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Wed, 28 Jun 2023 09:53:45 -0500 Subject: [PATCH 1/6] Replace thread-unsafe runtime requires with vanilla autoload 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 https://github.com/sparklemotion/http-cookie/issues/27 --- lib/http/cookie_jar.rb | 22 ++---------------- lib/http/cookie_jar/abstract_saver.rb | 33 +++++++++------------------ lib/http/cookie_jar/abstract_store.rb | 30 +++++++++--------------- test/test_http_cookie_jar.rb | 26 ++++----------------- 4 files changed, 28 insertions(+), 83 deletions(-) diff --git a/lib/http/cookie_jar.rb b/lib/http/cookie_jar.rb index ef5429b..74584fa 100644 --- a/lib/http/cookie_jar.rb +++ b/lib/http/cookie_jar.rb @@ -6,26 +6,8 @@ # any particular website. class HTTP::CookieJar - class << self - def const_missing(name) - case name.to_s - when /\A([A-Za-z]+)Store\z/ - file = 'http/cookie_jar/%s_store' % $1.downcase - when /\A([A-Za-z]+)Saver\z/ - file = 'http/cookie_jar/%s_saver' % $1.downcase - end - begin - require file - rescue LoadError - raise NameError, 'can\'t resolve constant %s; failed to load %s' % [name, file] - end - if const_defined?(name) - const_get(name) - else - raise NameError, 'can\'t resolve constant %s after loading %s' % [name, file] - end - end - end + autoload :AbstractStore, 'http/cookie_jar/abstract_store' + autoload :AbstractSaver, 'http/cookie_jar/abstract_saver' attr_reader :store diff --git a/lib/http/cookie_jar/abstract_saver.rb b/lib/http/cookie_jar/abstract_saver.rb index 9d82873..758f5e5 100644 --- a/lib/http/cookie_jar/abstract_saver.rb +++ b/lib/http/cookie_jar/abstract_saver.rb @@ -2,29 +2,15 @@ # An abstract superclass for all saver classes. class HTTP::CookieJar::AbstractSaver - class << self - @@class_map = {} - # Gets an implementation class by the name, optionally trying to - # load "http/cookie_jar/*_saver" if not found. If loading fails, - # IndexError is raised. - def implementation(symbol) - @@class_map.fetch(symbol) - rescue IndexError - begin - require 'http/cookie_jar/%s_saver' % symbol - @@class_map.fetch(symbol) - rescue LoadError, IndexError - raise IndexError, 'cookie saver unavailable: %s' % symbol.inspect - end - end - - def inherited(subclass) # :nodoc: - @@class_map[class_to_symbol(subclass)] = subclass - end - - def class_to_symbol(klass) # :nodoc: - klass.name[/[^:]+?(?=Saver$|$)/].downcase.to_sym + def self.implementation(symbol) + case symbol + when :yaml + HTTP::CookieJar::YAMLSaver + when :cookiestxt + HTTP::CookieJar::CookiestxtSaver + else + raise IndexError, 'cookie saver unavailable: %s' % symbol.inspect end end @@ -63,3 +49,6 @@ def load(io, jar) # self end end + +require "http/cookie_jar/yaml_saver" +require "http/cookie_jar/cookiestxt_saver" diff --git a/lib/http/cookie_jar/abstract_store.rb b/lib/http/cookie_jar/abstract_store.rb index 9c062ed..27941b7 100644 --- a/lib/http/cookie_jar/abstract_store.rb +++ b/lib/http/cookie_jar/abstract_store.rb @@ -6,29 +6,18 @@ class HTTP::CookieJar::AbstractStore include MonitorMixin class << self - @@class_map = {} - # Gets an implementation class by the name, optionally trying to - # load "http/cookie_jar/*_store" if not found. If loading fails, - # IndexError is raised. + # Gets an implementation class by the name. def implementation(symbol) - @@class_map.fetch(symbol) - rescue IndexError - begin - require 'http/cookie_jar/%s_store' % symbol - @@class_map.fetch(symbol) - rescue LoadError, IndexError => e - raise IndexError, 'cookie store unavailable: %s, error: %s' % symbol.inspect, e.message + case symbol + when :hash + HTTP::CookieJar::HashStore + when :mozilla + HTTP::CookieJar::MozillaStore + else + raise IndexError, 'cookie store unavailable: %s' % symbol.inspect end end - - def inherited(subclass) # :nodoc: - @@class_map[class_to_symbol(subclass)] = subclass - end - - def class_to_symbol(klass) # :nodoc: - klass.name[/[^:]+?(?=Store$|$)/].downcase.to_sym - end end # Defines options and their default values. @@ -122,3 +111,6 @@ def cleanup(session = false) # self end end + +require 'http/cookie_jar/hash_store' +require 'http/cookie_jar/mozilla_store' diff --git a/test/test_http_cookie_jar.rb b/test/test_http_cookie_jar.rb index 8f57abf..24a4e09 100644 --- a/test/test_http_cookie_jar.rb +++ b/test/test_http_cookie_jar.rb @@ -10,17 +10,8 @@ def test_nonexistent_store end def test_erroneous_store - Dir.mktmpdir { |dir| - Dir.mkdir(File.join(dir, 'http')) - Dir.mkdir(File.join(dir, 'http', 'cookie_jar')) - rb = File.join(dir, 'http', 'cookie_jar', 'erroneous_store.rb') - File.open(rb, 'w').close - $LOAD_PATH.unshift(dir) - - assert_raises(NameError) { - HTTP::CookieJar::ErroneousStore - } - assert($LOADED_FEATURES.any? { |file| FileTest.identical?(file, rb) }) + assert_raises(NameError) { + HTTP::CookieJar::ErroneousStore } end @@ -31,17 +22,8 @@ def test_nonexistent_saver end def test_erroneous_saver - Dir.mktmpdir { |dir| - Dir.mkdir(File.join(dir, 'http')) - Dir.mkdir(File.join(dir, 'http', 'cookie_jar')) - rb = File.join(dir, 'http', 'cookie_jar', 'erroneous_saver.rb') - File.open(rb, 'w').close - $LOAD_PATH.unshift(dir) - - assert_raises(NameError) { - HTTP::CookieJar::ErroneousSaver - } - assert($LOADED_FEATURES.any? { |file| FileTest.identical?(file, rb) }) + assert_raises(NameError) { + HTTP::CookieJar::ErroneousSaver } end end From 48ce2951190898a0c998b5db0e5ce58278fb8ff0 Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Wed, 28 Jun 2023 16:55:38 -0500 Subject: [PATCH 2/6] Further remove autoloading, the issue still repros on jruby and truffle 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
' $ 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 --- lib/http/cookie.rb | 5 +---- lib/http/cookie_jar.rb | 5 +++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/http/cookie.rb b/lib/http/cookie.rb index 01e8254..278036a 100644 --- a/lib/http/cookie.rb +++ b/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' -end +require 'http/cookie_jar' # This class is used to represent an HTTP Cookie. class HTTP::Cookie diff --git a/lib/http/cookie_jar.rb b/lib/http/cookie_jar.rb index 74584fa..01eef36 100644 --- a/lib/http/cookie_jar.rb +++ b/lib/http/cookie_jar.rb @@ -6,8 +6,6 @@ # any particular website. class HTTP::CookieJar - autoload :AbstractStore, 'http/cookie_jar/abstract_store' - autoload :AbstractSaver, 'http/cookie_jar/abstract_saver' attr_reader :store @@ -324,3 +322,6 @@ def cleanup(session = false) self end end + +require 'http/cookie_jar/abstract_store' +require 'http/cookie_jar/abstract_saver' From 20d992752cc76dc4e2119402e87ee20ed1874d12 Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Wed, 28 Jun 2023 17:41:27 -0500 Subject: [PATCH 3/6] Add jruby runtime exclusion to abstract store 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. --- lib/http/cookie_jar/abstract_store.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/http/cookie_jar/abstract_store.rb b/lib/http/cookie_jar/abstract_store.rb index 27941b7..f7cecd4 100644 --- a/lib/http/cookie_jar/abstract_store.rb +++ b/lib/http/cookie_jar/abstract_store.rb @@ -113,4 +113,15 @@ def cleanup(session = false) end require 'http/cookie_jar/hash_store' -require 'http/cookie_jar/mozilla_store' + +begin + require 'http/cookie_jar/mozilla_store' +rescue LoadError + if defined?(JRUBY_VERSION) + class HTTP::CookieJar::MozillaStore + def initialize(*) + raise ArgumentError, "MozillaStore is not supported on JRuby" + end + end + end +end From 401a002c918eebf986066247f392951b1399fa50 Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Fri, 30 Jun 2023 09:48:50 -0500 Subject: [PATCH 4/6] Remove circular requires :85: warning: :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 `
' 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
' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/test/test_http_cookie.rb:2:in `' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/test/helper.rb:4:in `' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/lib/http/cookie.rb:7:in `' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar.rb:325:in `' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar/abstract_store.rb:115:in `' from :85:in `require' from :85:in `require' from /home/cbrasic/repos/http-cookie/lib/http/cookie_jar/hash_store.rb:2:in `' from :85:in `require' from :85:in `require' --- lib/http/cookie_jar.rb | 1 - lib/http/cookie_jar/cookiestxt_saver.rb | 1 - lib/http/cookie_jar/hash_store.rb | 1 - lib/http/cookie_jar/mozilla_store.rb | 1 - lib/http/cookie_jar/yaml_saver.rb | 1 - 5 files changed, 5 deletions(-) diff --git a/lib/http/cookie_jar.rb b/lib/http/cookie_jar.rb index 01eef36..dd2faf7 100644 --- a/lib/http/cookie_jar.rb +++ b/lib/http/cookie_jar.rb @@ -1,5 +1,4 @@ # :markup: markdown -require 'http/cookie' ## # This class is used to manage the Cookies that have been returned from diff --git a/lib/http/cookie_jar/cookiestxt_saver.rb b/lib/http/cookie_jar/cookiestxt_saver.rb index df0ca01..7b0d57c 100644 --- a/lib/http/cookie_jar/cookiestxt_saver.rb +++ b/lib/http/cookie_jar/cookiestxt_saver.rb @@ -1,5 +1,4 @@ # :markup: markdown -require 'http/cookie_jar' # CookiestxtSaver saves and loads cookies in the cookies.txt format. class HTTP::CookieJar::CookiestxtSaver < HTTP::CookieJar::AbstractSaver diff --git a/lib/http/cookie_jar/hash_store.rb b/lib/http/cookie_jar/hash_store.rb index 258be46..cff4caa 100644 --- a/lib/http/cookie_jar/hash_store.rb +++ b/lib/http/cookie_jar/hash_store.rb @@ -1,5 +1,4 @@ # :markup: markdown -require 'http/cookie_jar' class HTTP::CookieJar # A store class that uses a hash-based cookie store. diff --git a/lib/http/cookie_jar/mozilla_store.rb b/lib/http/cookie_jar/mozilla_store.rb index 03433a7..fa37b68 100644 --- a/lib/http/cookie_jar/mozilla_store.rb +++ b/lib/http/cookie_jar/mozilla_store.rb @@ -1,5 +1,4 @@ # :markup: markdown -require 'http/cookie_jar' require 'sqlite3' class HTTP::CookieJar diff --git a/lib/http/cookie_jar/yaml_saver.rb b/lib/http/cookie_jar/yaml_saver.rb index bc83f04..7ca342e 100644 --- a/lib/http/cookie_jar/yaml_saver.rb +++ b/lib/http/cookie_jar/yaml_saver.rb @@ -1,5 +1,4 @@ # :markup: markdown -require 'http/cookie_jar' require 'psych' if !defined?(YAML) && RUBY_VERSION == "1.9.2" require 'yaml' From e8dcdb492a46c49049b30a4d399cdd3510e236e0 Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Mon, 3 Jul 2023 15:45:01 -0500 Subject: [PATCH 5/6] Ensure YAML/Psych/SQLite3 are not loaded unless they are used Per request here: https://github.com/sparklemotion/http-cookie/pull/43#pullrequestreview-1505731171 --- lib/http/cookie_jar/mozilla_store.rb | 2 +- lib/http/cookie_jar/yaml_saver.rb | 15 +++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/lib/http/cookie_jar/mozilla_store.rb b/lib/http/cookie_jar/mozilla_store.rb index fa37b68..ec52205 100644 --- a/lib/http/cookie_jar/mozilla_store.rb +++ b/lib/http/cookie_jar/mozilla_store.rb @@ -1,5 +1,5 @@ # :markup: markdown -require 'sqlite3' +autoload :SQLite3, 'sqlite3' class HTTP::CookieJar # A store class that uses Mozilla compatible SQLite3 database as diff --git a/lib/http/cookie_jar/yaml_saver.rb b/lib/http/cookie_jar/yaml_saver.rb index 7ca342e..1b362b6 100644 --- a/lib/http/cookie_jar/yaml_saver.rb +++ b/lib/http/cookie_jar/yaml_saver.rb @@ -1,6 +1,5 @@ # :markup: markdown -require 'psych' if !defined?(YAML) && RUBY_VERSION == "1.9.2" -require 'yaml' +autoload :YAML, 'yaml' # YAMLSaver saves and loads cookies in the YAML format. It can load a # YAML file saved by Mechanize, but the saving format is not @@ -73,13 +72,9 @@ def default_options {} end - if YAML.name == 'Psych' && Psych::VERSION >= '3.1' - def load_yaml(yaml) - YAML.safe_load(yaml, :permitted_classes => %w[Time HTTP::Cookie Mechanize::Cookie DomainName], :aliases => true) - end - else - def load_yaml(yaml) - YAML.load(yaml) - end + def load_yaml(yaml) + YAML.safe_load(yaml, :permitted_classes => %w[Time HTTP::Cookie Mechanize::Cookie DomainName], :aliases => true) + rescue NoMethodError # ruby < 2.0, no safe_load + YAML.load(yaml) end end From 5a71a8bf0021a6a54b0f31fcfc7a5369a168217e Mon Sep 17 00:00:00 2001 From: Carl Brasic Date: Mon, 3 Jul 2023 16:32:39 -0500 Subject: [PATCH 6/6] Skip loading sqlite3 entirely on jruby --- lib/http/cookie_jar/abstract_store.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/http/cookie_jar/abstract_store.rb b/lib/http/cookie_jar/abstract_store.rb index f7cecd4..773b081 100644 --- a/lib/http/cookie_jar/abstract_store.rb +++ b/lib/http/cookie_jar/abstract_store.rb @@ -114,14 +114,13 @@ def cleanup(session = false) require 'http/cookie_jar/hash_store' -begin - require 'http/cookie_jar/mozilla_store' -rescue LoadError - if defined?(JRUBY_VERSION) - class HTTP::CookieJar::MozillaStore - def initialize(*) - raise ArgumentError, "MozillaStore is not supported on JRuby" - end +# Skip loading MozillaStore on JRuby. +if defined?(JRUBY_VERSION) + class HTTP::CookieJar::MozillaStore + def initialize(*) + raise ArgumentError, "MozillaStore is not supported on JRuby" end end +else + require 'http/cookie_jar/mozilla_store' end