Skip to content

feature detect to make net/http usable with JRuby#52

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

Merged
jeremyevans merged 3 commits into from
Apr 20, 2022
Merged

feature detect to make net/http usable with JRuby #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0a27f1
[jruby] handle missing session_new_cb=
kares Apr 8, 2022
f87f906
[jruby] also do not call dummy session_cache_mode=
kares Apr 8, 2022
3647dbe
[test] handle/skip tests on JRuby
kares Apr 8, 2022
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
12 changes: 8 additions & 4 deletions lib/net/http.rb
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1033,10 +1033,14 @@ def connect
end
end
@ssl_context.set_params(ssl_parameters)
@ssl_context.session_cache_mode =
OpenSSL::SSL::SSLContext::SESSION_CACHE_CLIENT |
OpenSSL::SSL::SSLContext::SESSION_CACHE_NO_INTERNAL_STORE
@ssl_context.session_new_cb = proc {|sock, sess| @ssl_session = sess }
unless @ssl_context.session_cache_mode.nil? # a dummy method on JRuby

@jeremyevansjeremyevansApr 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If JRuby doesn't support @ssl_context.session_cache_mode, maybe don't define the method and use if defined?(@ssl_context.session_cache_mode).

@headiusheadiusApr 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, we probably should do that, but we implemented it as a no-op plus warning so we might be reluctant to remove the method all together.

@kareskaresApr 11, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

👍 the reasoning being session_cache_mode does not affect the user-code much overall and thus it existed for some time and removing it might be considered breaking ... thus the nil compromise will probably stay with us.
smt like session_new_cb, while being "low" level, tends to get much more critical in user-code - if we had it as a dummy attr and the callback never fired it would be counter productive ...

@ssl_context.session_cache_mode =
OpenSSL::SSL::SSLContext::SESSION_CACHE_CLIENT |
OpenSSL::SSL::SSLContext::SESSION_CACHE_NO_INTERNAL_STORE
end
if @ssl_context.respond_to?(:session_new_cb) # not implemented under JRuby
@ssl_context.session_new_cb = proc {|sock, sess| @ssl_session = sess }
end

# Still do the post_connection_check below even if connecting
# to IP address
Expand Down
8 changes: 5 additions & 3 deletions test/net/http/test_https.rb
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -152,12 +152,14 @@ def test_session_reuse
end

http.start
assert_equal false, http.instance_variable_get(:@socket).io.session_reused?
session_reused = http.instance_variable_get(:@socket).io.session_reused?
assert_false session_reused unless session_reused.nil? # can not detect re-use under JRuby
http.get("/")
http.finish

http.start
assert_equal true, http.instance_variable_get(:@socket).io.session_reused?
session_reused = http.instance_variable_get(:@socket).io.session_reused?
assert_true session_reused unless session_reused.nil? # can not detect re-use under JRuby
assert_equal $test_net_http_data, http.get("/").body
http.finish
end
Expand DownExpand Up@@ -301,7 +303,7 @@ def test_max_version
ex = assert_raise(OpenSSL::SSL::SSLError){
http.request_get("/") {|res| }
}
re_msg = /\ASSL_connect returned=1 errno=0 |SSL_CTX_set_max_proto_version/
re_msg = /\ASSL_connect returned=1 errno=0 |SSL_CTX_set_max_proto_version|No appropriate protocol/
assert_match(re_msg, ex.message)
end

Expand Down