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

Backport PR #14175 to 8.3: Refactor: avoid loading polyglot #14227

Merged
merged 1 commit into from Jun 8, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jun 7, 2022

Backport PR #14175 to 8.3 branch, original message:


Release notes

[rn:skip]

What does this PR do?

Avoids require 'polyglot' (and a few others) which patches Kernel.require, for which we introduced another patch due JRuby 9.3 in #14114 (targeted for LS 8.3).

Why is it important/What is the impact to the user?

No user impact.

With this patch in place logstash-core in tests no longer loads polyglot gem (which patches Kernel.require and thus leads to a circular error issue - same as observed in #14114) and is easier to test against JRuby 9.3 (outside Docker).

Sample failure:

An error occurred while loading ./spec/inputs/http_poller_spec.rb.
Failure/Error: require "logstash/devutils/rspec/spec_helper"

RuntimeError:
  circular causes
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:73:in `require'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-1.10.0/lib/faraday/dependency_loader.rb:11:in `dependency'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-excon-1.1.0/lib/faraday/adapter/excon.rb:7:in `<class:Excon>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-excon-1.1.0/lib/faraday/adapter/excon.rb:6:in `<class:Adapter>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-excon-1.1.0/lib/faraday/adapter/excon.rb:4:in `<module:Faraday>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-excon-1.1.0/lib/faraday/adapter/excon.rb:3:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-excon-1.1.0/lib/faraday/excon.rb:3:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/faraday-1.10.0/lib/faraday.rb:40:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/elasticsearch-transport-7.17.1/lib/elasticsearch/transport.rb:23:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/elasticsearch-7.17.1/lib/elasticsearch.rb:19:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /home/kares/workspace/work/elastic/logstash-7.16.3/logstash-core/lib/logstash/elasticsearch_client.rb:18:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /home/kares/workspace/work/elastic/logstash-7.16.3/logstash-core/lib/logstash/config/modules_common.rb:18:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /home/kares/workspace/work/elastic/logstash-7.16.3/logstash-core/lib/logstash/config/source/modules.rb:19:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /home/kares/workspace/work/elastic/logstash-7.16.3/logstash-core/lib/logstash/config/source_loader.rb:19:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
# /home/kares/workspace/work/elastic/logstash-7.16.3/logstash-core/lib/logstash/agent.rb:24:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/logstash-devutils-2.3.0-java/lib/logstash/devutils/rspec/logstash_helpers.rb:1:in `<main>'
# /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/logstash-devutils-2.3.0-java/lib/logstash/devutils/rspec/spec_helper.rb:17:in `<main>'


Tested with a sample plugin doing a runtime require (just to demonstrate Polyglot is no longer being loaded during plugin's tests):

        begin
          require 'foo.rb'
        rescue LoadError => e
          puts e.inspect
          puts '  ' + e.backtrace.join("\n  ")
        end

WITHOUT this patch (polyglot is still involved with require):

#<Polyglot::PolyglotLoadError: Failed to load foo.rb using extensions rb, treetop, tt>
  /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:55:in `load'
  /opt/local/rvm/gems/jruby-9.3.4.0@ls/gems/polyglot-0.3.5/lib/polyglot.rb:68:in `require'
  /home/kares/workspace/work/elastic/logstash/logstash-core/lib/logstash/patches/polyglot.rb:9:in `require'
  /home/kares/workspace/work/elastic/plugins/logstash-input-elasticsearch/spec/inputs/elasticsearch_spec.rb:42:in `block in <main>'
  org/jruby/RubyBasicObject.java:2673:in `instance_exec'

WITH a patched LS :

#<LoadError: no such file to load -- foo.rb>
  org/jruby/RubyKernel.java:1017:in `require'
  /home/kares/workspace/work/elastic/plugins/logstash-input-elasticsearch/spec/inputs/elasticsearch_spec.rb:42:in `block in <main>'
  org/jruby/RubyBasicObject.java:2673:in `instance_exec'

Checklist

Integration tests are currently failing due missing SNAPSHOT builds.
Here's the same change set (+ #14184) against 8.3 #14182 🟢

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

* Refactor: require treetop/runtime - avoids loading polyglot
* Build: instruct Bundler not to auto-load polyglot/treetop

+ Build: these deps are properly required as needed
all of them only used in one place (outside of normal bootstrap)

(cherry picked from commit 433b341)
@jsvd jsvd merged commit 6036b3d into 8.3 Jun 8, 2022
@jsvd jsvd deleted the backport_14175_8.3 branch June 8, 2022 11:29
robbavey added a commit to robbavey/logstash that referenced this pull request Jun 20, 2022
robbavey added a commit that referenced this pull request Jun 20, 2022
This reverts the upgrade to jruby-9.3.4.0

Unfortunately, jruby-9.3.4.0 has some significant issues which may impact the running of Logstash in productions:

Bugs we believe we can mitigate and have provided workarounds and fixes for:
- jruby/jruby#7246 may cause a pipeline to crash if a Java functional interface is called
  by a block.

Bugs we believe we have workarounds for:
- jruby/jruby#7244 - binding Java methods in 9.3 breaks compatibility

Bugs that we expect may impact users, but for which, we do not currently have a mitigation or workaround:
- jruby/jruby#7229 - Memory regression from 9.2.20.1 to 9.3.0.0

* Revert "Refactor: avoid loading polyglot (#14175) (#14227)"
This reverts commit 6036b3d.

* Revert "Update jruby version to `9.3.4.0` (#14114)"
This reverts commit 4a2268a.

* Roll back version of nokogiri to previous version compatible with jruby < 2.6
This is a manual rollback
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

3 participants