Removed static part from PluginRegistry to avoid static initializer #12799

andsel · 2021-04-06T12:58:00Z

What does this PR do?

Removes the need of static initializer and static methods in org.logstash.plugins.discovery.PluginRegistry
Changed org.logstash.plugins.PluginLookup to implement org.logstash.plugins.factory.PluginFactoryExt.PluginResolver

These changes are preparatory to adding an "alias resolver" during plugin lookup #12796

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

Static initializers and static method made test impossible when a dependency is static instead of customizable.
Other than this, the construction sequence is made explicit, describing better the chain of dependency of various services.

Checklist

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

Author's Checklist

[ x] Run Logstash to laod a pipeline configuration with and without Java native plugins. Java native plugins are those loaded by Java's PluginRegistry.

How to test this PR locally

Related issues

Relates Load a plugin by alias name. #12796

andsel · 2021-04-07T12:06:59Z

Jenkins test this please

andsel · 2021-04-07T12:36:48Z

Jenkins test this please

kares

👍 nice to see less static state

have a question about the getInstance factory - whether we could simplify (avoid) the singleton 'thread-safe' initialization.

kares · 2021-04-12T07:21:38Z

logstash-core/src/main/java/org/logstash/plugins/discovery/PluginRegistry.java

- * Registry for built-in Java plugins (not installed via logstash-plugin)
- */
+ * Registry for built-in Java plugins (not installed via logstash-plugin).
+ * This is singleton ofr two reasons:


Suggested change

* This is singleton ofr two reasons:

* This is singleton for two reasons:

kares · 2021-04-12T07:24:44Z

logstash-core/src/main/java/org/logstash/plugins/discovery/PluginRegistry.java


-    private PluginRegistry() {} // utility class
-
+    public static PluginRegistry getInstance() {


do we really need the lazy initialization here? (cause I think it should be marked volatile otherwise)
couldn't we just have a private static final PluginRegistry INSTANCE = new PluginRegistry() ?
(the class will initialize on it's first use, or if that is an issue let's have a nested static class to hold the INSTANCE)

The problem with private static final PluginRegistry INSTANCE = new PluginRegistry() is that statement is executed when the class is loaded, and is really similar to have a static block that does all the initialization. The problem with this, is that we don't have direct control of "when" the PluginRegistry is instantiated, but the order is controlled by import/require sequence.
Having this control could be helpful in writing tests for the alias part (I hope).

Regarding the volatile, if we used it we could potentially have many threads (when we have multiple pipelines) that creates more instances, but then only one is saved and used by the other.
Creating multiple instances could raise some problems with the reflections library, because these 2 lines:

Reflections reflections = new Reflections("org.logstash.plugins"); Set<Class<?>> annotated = reflections.getTypesAnnotatedWith(LogstashPlugin.class);

must be executed in single thread, and previously this condition was granted by the static {} initializer that's executed by the class loading mechanism and that's granted to be single thread, else we risk problems that one threads executing reflections closes the jar file to the other thread that's executing the classpath scan. It could raise problems like:

[DEBUG] 2021-04-08 09:21:08.935 [Agent thread] agent - Converging pipelines state {:actions_count=>2} [DEBUG] 2021-04-08 09:21:08.944 [Converge PipelineAction::Create<test>] agent - Executing action {:action=>LogStash::PipelineAction::Create/pipeline_id:test} [DEBUG] 2021-04-08 09:21:08.944 [Converge PipelineAction::Create<test2>] agent - Executing action {:action=>LogStash::PipelineAction::Create/pipeline_id:test2} [INFO ] 2021-04-08 09:21:09.094 [Api Webserver] agent - Successfully started Logstash API endpoint {:port=>9600} [DEBUG] 2021-04-08 09:21:09.358 [Converge PipelineAction::Create<test>] Reflections - going to scan these urls: jar:file:/home/andrea/workspace/logstash_andsel/build/logstash-8.0.0-SNAPSHOT/logstash-core/lib/jars/logstash-core.jar!/ [DEBUG] 2021-04-08 09:21:09.358 [Converge PipelineAction::Create<test2>] Reflections - going to scan these urls: jar:file:/home/andrea/workspace/logstash_andsel/build/logstash-8.0.0-SNAPSHOT/logstash-core/lib/jars/logstash-core.jar!/ [INFO ] 2021-04-08 09:21:09.395 [Converge PipelineAction::Create<test>] Reflections - Reflections took 35 ms to scan 1 urls, producing 24 keys and 48 values [ERROR] 2021-04-08 09:21:09.397 [Converge PipelineAction::Create<test2>] agent - Failed to execute action { :action=>LogStash::PipelineAction::Create/pipeline_id:test2, :exception=>"Java::JavaLang::IllegalStateException", :message=>"zip file closed", :backtrace=>["java.base/java.util.zip.ZipFile.ensureOpen(ZipFile.java:915)", "java.base/java.util.zip.ZipFile$ZipEntryIterator.next(ZipFile.java:525)", "java.base/java.util.zip.ZipFile$ZipEntryIterator.nextElement(ZipFile.java:518)", "java.base/java.util.zip.ZipFile$ZipEntryIterator.nextElement(ZipFile.java:494)", "org.reflections.vfs.ZipDir$1$1.computeNext(ZipDir.java:31)", "org.reflections.vfs.ZipDir$1$1.computeNext(ZipDir.java:26)", "com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:141)", "com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:136)", "org.reflections.Reflections.scan(Reflections.java:243)", "org.reflections.Reflections.scan(Reflections.java:202)", "org.reflections.Reflections.<init>(Reflections.java:123)", "org.reflections.Reflections.<init>(Reflections.java:168)", "org.reflections.Reflections.<init>(Reflections.java:141)", "org.logstash.plugins.discovery.PluginRegistry.discoverPlugins(PluginRegistry.java:55)", "org.logstash.plugins.discovery.PluginRegistry.<init>(PluginRegistry.java:50)", "org.logstash.plugins.factory.PluginFactoryExt.<init>(PluginFactoryExt.java:85)", "org.logstash.execution.JavaBasePipelineExt.initialize(JavaBasePipelineExt.java:73)", "org.logstash.execution.JavaBasePipelineExt$INVOKER$i$1$0$initialize.call(JavaBasePipelineExt$INVOKER$i$1$0$initialize.gen)", "org.jruby.internal.runtime.methods.JavaMethod$JavaMethodN.call(JavaMethod.java:837)", "org.jruby.ir.runtime.IRRuntimeHelpers.instanceSuper(IRRuntimeHelpers.java:1169)", "org.jruby.ir.runtime.IRRuntimeHelpers.instanceSuperSplatArgs(IRRuntimeHelpers.java:1156)", "org.jruby.ir.targets.InstanceSuperInvokeSite.invoke(InstanceSuperInvokeSite.java:39)", "home.andrea.workspace.logstash_andsel.build.logstash_minus_8_dot_0_dot_0_minus_SNAPSHOT.logstash_minus_core.lib.logstash.java_pipeline.RUBY$method$initialize$0(/home/andrea/workspace/logstash_andsel/build/logstash-8.0.0-SNAPSHOT/logstash-core/lib/logstash/java_pipeline.rb:47)", "org.jruby.internal.runtime.methods.CompiledIRMethod.call(CompiledIRMethod.java:80)", "org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:70)", "org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(CachingCallSite.java:332)", "org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:86)", "org.jruby.RubyClass.newInstance(RubyClass.java:939)", "org.jruby.RubyClass$INVOKER$i$newInstance.call(RubyClass$INVOKER$i$newInstance.gen)", "org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(CachingCallSite.java:332)", "org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:86)", "org.jruby.ir.instructions.CallBase.interpret(CallBase.java:549)", "org.jruby.ir.interpreter.InterpreterEngine.processCall(InterpreterEngine.java:361)", "org.jruby.ir.interpreter.StartupInterpreterEngine.interpret(StartupInterpreterEngine.java:72)", "org.jruby.internal.runtime.methods.MixedModeIRMethod.INTERPRET_METHOD(MixedModeIRMethod.java:86)", "org.jruby.internal.runtime.methods.MixedModeIRMethod.call(MixedModeIRMethod.java:73)", "org.jruby.ir.targets.InvokeSite.invoke(InvokeSite.java:207)", "home.andrea.workspace.logstash_andsel.build.logstash_minus_8_dot_0_dot_0_minus_SNAPSHOT.logstash_minus_core.lib.logstash.agent.RUBY$block$converge_state$2(/home/andrea/workspace/logstash_andsel/build/logstash-8.0.0-SNAPSHOT/logstash-core/lib/logstash/agent.rb:382)", "org.jruby.runtime.CompiledIRBlockBody.callDirect(CompiledIRBlockBody.java:138)", "org.jruby.runtime.IRBlockBody.call(IRBlockBody.java:58)", "org.jruby.runtime.IRBlockBody.call(IRBlockBody.java:52)", "org.jruby.runtime.Block.call(Block.java:139)", "org.jruby.RubyProc.call(RubyProc.java:318)", "org.jruby.internal.runtime.RubyRunnable.run(RubyRunnable.java:105)", "java.base/java.lang.Thread.run(Thread.java:834)"]}

OK, thanks for the explanation.

(cause I think it should be marked volatile otherwise)

What I meant is to mark the static instance to prevent issues like "cache incoherence" (some read)

private static volatile PluginRegistry instance; public static PluginRegistry getInstance() { PluginRegistry instance = PluginRegistry.instance; if (instance == null) { synchronized (PluginRegistry.class) { instance = PluginRegistry.instance; if (instance == null) { instance = PluginRegistry.instance = new PluginRegistry(); } } } return instance; }

kares

LGTM

andsel · 2021-04-12T09:44:13Z

Jenkins test this please

…hread safe in jar archives scan

… instance

andsel · 2021-04-13T13:17:37Z

Jenkins test this please

…lastic#12799) Removed static part from PluginRegistry to avoid static initializer, transforming it into a singleton. Reflections library is not thread safe in jar archives scan, so there must be only one instance of PluginRegistry

Introcuce the concept of alias for a plugin (#12796) and removes static part from PluginRegistry to avoid static initializer (#12799) Creates an AliasRegistry to map plugin aliases to original plugins. If a real plugin with same name of the an alias is present in the system, then the real plugin take precedence during the instantiation of the pipeline. Simplified the error handling in class lookup Co-authored-by: Ry Biesemeyer <[email protected]>

andsel added the enhancement label Apr 6, 2021

roaksoax added status:needs-review and removed status:needs-review labels Apr 6, 2021

andsel mentioned this pull request Apr 7, 2021

[Meta] Add plugin aliasing feature #12793

Closed

9 tasks

andsel force-pushed the fix/avoid_static_in_plugin_lookup branch from 2d43927 to ecab552 Compare April 8, 2021 16:23

elasticsearch-bot self-assigned this Apr 12, 2021

kares reviewed Apr 12, 2021

View reviewed changes

kares unassigned elasticsearch-bot Apr 12, 2021

kares approved these changes Apr 12, 2021

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Apr 12, 2021

andsel added 4 commits April 13, 2021 14:21

Removed static part from PluginRegistry to avoid static initializer

41c0995

Switched PluginRegistry to be singleton, because Reflections is not t…

450459a

…hread safe in jar archives scan

Added volatile to publish the shared state contained in the singleton…

cb45fd1

… instance

Minor, just capitalized the static variables

e6dd798

andsel force-pushed the fix/avoid_static_in_plugin_lookup branch from f6e790b to e6dd798 Compare April 13, 2021 12:21

andsel merged commit bb0074f into elastic:master Apr 13, 2021

andsel added the v8.0.0 label Apr 13, 2021

andsel mentioned this pull request Apr 20, 2021

[Backport 7.x] plugin alias handling in logstash-core #12835

Merged

andsel added the v7.13.0 label Apr 20, 2021

roaksoax removed the status:approved label Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removed static part from PluginRegistry to avoid static initializer #12799

Removed static part from PluginRegistry to avoid static initializer #12799

Uh oh!

andsel commented Apr 6, 2021

Uh oh!

andsel commented Apr 7, 2021

Uh oh!

andsel commented Apr 7, 2021

Uh oh!

kares left a comment

Uh oh!

kares Apr 12, 2021

Uh oh!

kares Apr 12, 2021

Uh oh!

andsel Apr 12, 2021

Uh oh!

kares Apr 12, 2021

Uh oh!

kares left a comment

Uh oh!

andsel commented Apr 12, 2021

Uh oh!

andsel commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	* This is singleton ofr two reasons:
	* This is singleton for two reasons:


		private PluginRegistry() {} // utility class

		public static PluginRegistry getInstance() {

Removed static part from PluginRegistry to avoid static initializer #12799

Removed static part from PluginRegistry to avoid static initializer #12799

Uh oh!

Conversation

andsel commented Apr 6, 2021

What does this PR do?

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

Checklist

Author's Checklist

How to test this PR locally

Related issues

Uh oh!

andsel commented Apr 7, 2021

Uh oh!

andsel commented Apr 7, 2021

Uh oh!

kares left a comment

Choose a reason for hiding this comment

Uh oh!

kares Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

kares Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

andsel Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

kares Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

kares left a comment

Choose a reason for hiding this comment

Uh oh!

andsel commented Apr 12, 2021

Uh oh!

andsel commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants