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

Lazily load openssl #3850

Merged
merged 4 commits into from Sep 17, 2020
Merged

Lazily load openssl #3850

merged 4 commits into from Sep 17, 2020

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Jul 24, 2020

Description:

This is a followup to #3809, and all the other PRs linked to that trying to deal with jruby load service issues.

To recap the issue, as first mentioned here, apparently on jruby sometimes autoloading a constant and making it available through require can lead to weird concurrency issues that result in the constant not being available in the end.

So the solution we've been proposing for these issue is to not do that, and either require or autoload, but not both.

Everything seems good now, except for OpenSSL constants, where we sometimes still get errors. In #3809 I unified our code to always require instead of autoloading openssl. However, the problem still reproduces on our latest code. See https://github.com/rubygems/rubygems/pull/3867/checks?check_run_id=1059447242, for example:

 NameError: uninitialized constant Gem::Security::OpenSSL
         org/jruby/RubyModule.java:3760:in `const_missing'
         /home/runner/work/rubygems/rubygems/lib/rubygems/security.rb:452:in `create_digest'
         /home/runner/work/rubygems/rubygems/lib/rubygems/package.rb:361:in `block in digest'
         org/jruby/RubyArray.java:1809:in `each'
         (...)

After closer inspection, I noticed that the constant was still getting autoloaded here: https://github.com/ruby/ruby/blob/93b78abd774109d1333d59eaf439b2e69ed0fe00/lib/net/http.rb#L25. And we can't really avoid that unless we stop requiring net/http and vendor it instead.

So my approach was to instead of requiring openssl always, autoload it always. Other than potentially fixing the bug, this has two extra benefits:

  • We can unde one monkey-patch to the net-http-persistent gem.
  • It should be more efficient.

So I'd like to try this approach.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@deivid-rodriguez deivid-rodriguez force-pushed the openssl_one_more branch 2 times, most recently from 2e1f8e3 to a042402 Compare July 24, 2020 10:01
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review September 2, 2020 12:25
@deivid-rodriguez
Copy link
Member Author

I filled the description of this PR and marked it as ready!

I believe the intention here is that the `OpenSSL` constant is
autoloaded if not yet required. Just checking `defined?` on it won't do
this.
We're still running into the `jruby` bug where the `OpenSSL` constant
can't be found even if it should be there. My hypotheses is that this is
happening because `net/http` still sets up an `autoload` for it. Since we
don't control that code, we can't really remove that `autoload`, so I'm
guessing doing the reserve (only `autoload` it and never explicitly
require it) should do the trick as well?
I found this file, which was used in the past to simulate openssl not
being available. It seems better than the current hack because it
doesn't require renaming system files which is quite invasive.
@deivid-rodriguez
Copy link
Member Author

I didn't see the jruby crashes again since the last time, but since this has other benefits I decided to go ahead with this. If it causes any issues, I'll revert it.

@deivid-rodriguez deivid-rodriguez merged commit 8fd5dee into master Sep 17, 2020
@deivid-rodriguez deivid-rodriguez deleted the openssl_one_more branch September 17, 2020 10:02
@deivid-rodriguez deivid-rodriguez changed the title Try to fix openssl availability issues on jruby Lazily load openssl Dec 7, 2020
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Try to fix openssl availability issues on jruby

(cherry picked from commit 8fd5dee)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant