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

Fix to support Ruby 3.0 Ractor #22

Merged
merged 1 commit into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion lib/uri.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
# module URI
# class RSYNC < Generic
# DEFAULT_PORT = 873
# URI.refresh_scheme_list
# end
# @@schemes['RSYNC'] = RSYNC
# end
# #=> URI::RSYNC
#
Expand Down
32 changes: 25 additions & 7 deletions lib/uri/common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ module URI
REGEXP = RFC2396_REGEXP
Parser = RFC2396_Parser
RFC3986_PARSER = RFC3986_Parser.new
Ractor.make_shareable(RFC3986_PARSER) if defined?(Ractor)

# URI::Parser.new
DEFAULT_PARSER = Parser.new
Expand All @@ -27,6 +28,7 @@ module URI
DEFAULT_PARSER.regexp.each_pair do |sym, str|
const_set(sym, str)
end
Ractor.make_shareable(DEFAULT_PARSER) if defined?(Ractor)

module Util # :nodoc:
def make_components_hash(klass, array_hash)
Expand Down Expand Up @@ -62,22 +64,38 @@ def make_components_hash(klass, array_hash)

include REGEXP

@@schemes = {}
SCHEME_LIST_MUTEX = Mutex.new
private_constant :SCHEME_LIST_MUTEX

# Returns a Hash of the defined schemes.
# The list is lazily calculated.
def self.scheme_list
@@schemes
return const_get(:SCHEMES) if defined?(SCHEMES)

SCHEME_LIST_MUTEX.synchronize do
const_set(:SCHEMES, ObjectSpace.
each_object(Class).
select { |klass| klass < URI::Generic }.
Copy link
Member

Choose a reason for hiding this comment

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

This seems very expensive, iterate the whole heap when previously it was just registered directly.
Could the Class#inherited hook be used instead at least?
Or maybe use Class#descendants (checking if available with respond_to?) from https://bugs.ruby-lang.org/issues/14394?

In #15 I think the way was clearly to use instance variables for this, and that has been accepted in https://bugs.ruby-lang.org/issues/17592. And the main Ractor could simply write to that instance variable. That has not been implemented yet though (cc @ko1).

Another strategy might be to haves SCHEMES = {}, and then once the first URI is created, Ractor.make_shareable(SCHEMES) unless SCHEMES.frozen?. That would prevent adding new schemes after the first URI is created, but maybe it's good enough, and it feels like it fits well with the Ractor model.

Adding support for Ractor by making everything slower/more hacky is IMHO absolutely not acceptable.

each_with_object({}) { |klass, acc| acc[klass.name.split('::').last.upcase] = klass }.
freeze)
end
end

# Re-calculate scheme list
def self.refresh_scheme_list
Copy link
Member

Choose a reason for hiding this comment

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

This is yet another hack, now the API exposes the caching to the end user, that's leaking implementation details.
It's ugly, there is no other word.

SCHEME_LIST_MUTEX.synchronize do
remove_const(:SCHEMES) if defined?(SCHEMES)
end

scheme_list
end

#
# Construct a URI instance, using the scheme to detect the appropriate class
# from +URI.scheme_list+.
#
def self.for(scheme, *arguments, default: Generic)
if scheme
uri_class = @@schemes[scheme.upcase] || default
else
uri_class = default
end
uri_class = scheme_list[scheme.to_s.upcase] || default

return uri_class.new(scheme, *arguments)
end
Expand Down
2 changes: 0 additions & 2 deletions lib/uri/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,4 @@ def set_user(v)
def set_password(v)
end
end

@@schemes['FILE'] = File
end
1 change: 0 additions & 1 deletion lib/uri/ftp.rb
Original file line number Diff line number Diff line change
Expand Up @@ -262,5 +262,4 @@ def to_s
return str
end
end
@@schemes['FTP'] = FTP
end
3 changes: 0 additions & 3 deletions lib/uri/http.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,4 @@ def request_uri
url.start_with?(?/.freeze) ? url : ?/ + url
end
end

@@schemes['HTTP'] = HTTP

end
1 change: 0 additions & 1 deletion lib/uri/https.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,4 @@ class HTTPS < HTTP
# A Default port of 443 for URI::HTTPS
DEFAULT_PORT = 443
end
@@schemes['HTTPS'] = HTTPS
end
2 changes: 0 additions & 2 deletions lib/uri/ldap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,4 @@ def hierarchical?
false
end
end

@@schemes['LDAP'] = LDAP
end
1 change: 0 additions & 1 deletion lib/uri/ldaps.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,4 @@ class LDAPS < LDAP
# A Default port of 636 for URI::LDAPS
DEFAULT_PORT = 636
end
@@schemes['LDAPS'] = LDAPS
end
2 changes: 0 additions & 2 deletions lib/uri/mailto.rb
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,4 @@ def to_mailtext
end
alias to_rfc822text to_mailtext
end

@@schemes['MAILTO'] = MailTo
end
3 changes: 0 additions & 3 deletions lib/uri/ws.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,4 @@ def request_uri
url.start_with?(?/.freeze) ? url : ?/ + url
end
end

@@schemes['WS'] = WS

end
1 change: 0 additions & 1 deletion lib/uri/wss.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,4 @@ class WSS < WS
# A Default port of 443 for URI::WSS
DEFAULT_PORT = 443
end
@@schemes['WSS'] = WSS
end
19 changes: 19 additions & 0 deletions test/uri/test_common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,25 @@ def test_extract
end
end

def test_ractor
return unless defined?(Ractor)
r = Ractor.new { URI.parse("https://ruby-lang.org/").inspect }
assert_equal(URI.parse("https://ruby-lang.org/").inspect, r.take)
end

def test_register_scheme
assert_equal(["FILE", "FTP", "HTTP", "HTTPS", "LDAP", "LDAPS", "MAILTO", "WS"].sort, URI.scheme_list.keys.sort)

begin
URI::Generic.const_set :FOOBAR, Class.new(URI::Generic)
URI.refresh_scheme_list
assert_equal(["FILE", "FTP", "HTTP", "HTTPS", "LDAP", "LDAPS", "MAILTO", "WS", "FOOBAR"].sort, URI.scheme_list.keys.sort)
ensure
URI::Generic.send(:remove_const, :FOOBAR)
URI.refresh_scheme_list
end
end

def test_regexp
EnvUtil.suppress_warning do
assert_instance_of Regexp, URI.regexp
Expand Down