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

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

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Apr 8, 2022

up till JRuby 9.3 (Ruby 2.6 compatibility) JRuby shipped a slightly patched version of net/http to work-around some of the limitations - mostly of JRuby-OpenSSL which is implemented on top of Java APIs (does not use OpenSSL).

for JRuby 9.4, which is in development atm, it would be nice to be able to ship the gem version of net/http - the changes here represent an effort to make things compatible, there's really one important change:

  • SSLContext#session_new_cb in not implemented under JRuby, it could be a dummy no-op but this would confuse users as these session_xxx callbacks do not get actually called (no way to hook into session management on the Java SSL engine end)
  • SSLContext#session_cache_mode= historically exist in JRuby but is effectively a no-op and issues a Kernel.warn, the SSLContext#session_cache_mode reader always returns nil as an attempt to be able to "feature-detect"
  • also looked into low hanging fruit in some of the tests (with these there's only 3 failures left under JRuby)

kares added 3 commits April 8, 2022 11:54
JRuby can not implement these using available SSL engine APIs
as it issues a warn-ing message under JRuby
@kares kares marked this pull request as ready for review April 8, 2022 11:32
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

This looks OK to me, though I don't have a good knowledge of JRuby's openssl extension.

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
Copy link
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).

Copy link
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.

Copy link
Contributor Author

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

@headius
Copy link
Contributor

headius commented Apr 13, 2022

I think this is ready to merge. It would be helpful to us completing work on JRuby 9.4 if we could get this into a gem (possibly prerelease).

@headius headius mentioned this pull request Apr 13, 2022
4 tasks
Copy link
Member

@nurse nurse left a comment

Choose a reason for hiding this comment

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

Generally it looks good to me.
@jeremyevans, could you merge this if you also think loos good

@jeremyevans jeremyevans merged commit 3237ef4 into ruby:master Apr 20, 2022
@headius
Copy link
Contributor

headius commented Apr 21, 2022

Can we get a gem pushed (preview is ok) so we can hook it up to JRuby's build and start using the gem?

@headius
Copy link
Contributor

headius commented Apr 26, 2022

Ping @nurse @hsbt can we get a gem release or preview gem release with this change?

@hsbt
Copy link
Member

hsbt commented Apr 27, 2022

@headius I send an invitation of net-http owner. Can you confirm it?

@headius
Copy link
Contributor

headius commented Apr 27, 2022

@hsbt thank you! I will push a pre gem today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants