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.

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

Jump to bottom

Remove thread-unsafe runtime requires #43

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+46 −103
Open

Remove thread-unsafe runtime requires #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf6391b
Replace thread-unsafe runtime requires with vanilla autoload
brasic Jun 28, 2023
48ce295
Further remove autoloading, the issue still repros on jruby and truffle
brasic Jun 28, 2023
20d9927
Add jruby runtime exclusion to abstract store
brasic Jun 28, 2023
401a002
Remove circular requires
brasic Jun 30, 2023
e8dcdb4
Ensure YAML/Psych/SQLite3 are not loaded unless they are used
brasic Jul 3, 2023
5a71a8b
Skip loading sqlite3 entirely on jruby
brasic Jul 3, 2023
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
5 changes: 1 addition & 4 deletions lib/http/cookie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

end
require 'http/cookie_jar'

# This class is used to represent an HTTP Cookie.
class HTTP::Cookie
Expand Down
24 changes: 3 additions & 21 deletions lib/http/cookie_jar.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,10 @@
# :markup: markdown
require 'http/cookie'

##
# This class is used to manage the Cookies that have been returned from
# 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

attr_reader :store

Expand Down Expand Up @@ -342,3 +321,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.

33 changes: 11 additions & 22 deletions lib/http/cookie_jar/abstract_saver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -63,3 +49,6 @@ def load(io, jar)
# self
end
end

require "http/cookie_jar/yaml_saver"
require "http/cookie_jar/cookiestxt_saver"
40 changes: 21 additions & 19 deletions lib/http/cookie_jar/abstract_store.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -122,3 +111,16 @@ def cleanup(session = false)
# self
end
end

require 'http/cookie_jar/hash_store'

# Skip loading MozillaStore on JRuby.
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.

end
end
else
require 'http/cookie_jar/mozilla_store'
end
1 change: 0 additions & 1 deletion lib/http/cookie_jar/cookiestxt_saver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
1 change: 0 additions & 1 deletion lib/http/cookie_jar/hash_store.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
# :markup: markdown
require 'http/cookie_jar'

class HTTP::CookieJar
# A store class that uses a hash-based cookie store.
Expand Down
3 changes: 1 addition & 2 deletions lib/http/cookie_jar/mozilla_store.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# :markup: markdown
require 'http/cookie_jar'
require 'sqlite3'
autoload :SQLite3, 'sqlite3'

class HTTP::CookieJar
# A store class that uses Mozilla compatible SQLite3 database as
Expand Down
16 changes: 5 additions & 11 deletions lib/http/cookie_jar/yaml_saver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
# :markup: markdown
require 'http/cookie_jar'
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
Expand Down Expand Up @@ -74,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)
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.

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
26 changes: 4 additions & 22 deletions test/test_http_cookie_jar.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down