Skip to content

Commit

Permalink
Make AliasRegistry a singleton (#14002)
Browse files Browse the repository at this point in the history
* Make AliasRegistry a singleton

The current implementation of AliasRegistry will create an instance of the Alias Registry for each
pipeline, which appears to potentially result in situations, such as in #13996, where multiple pipelines
are simultaneously loading an alias registry from a yaml file in the jar file using getResourceAsStream, with
the potential of the first thread closing the jar file underneath subsequent threads, leading to errors when
reading the yaml file as the JarFile has been closed after the initial thread completed accessing.

This commit changes the AliasRegistry to be a singleton, as is the PluginRegistry.

Relates: #13996, #13666

* Update reflections library to 0.9.12 to avoid multi threading bug

Earlier versions of the reflections library used in the plugin registry would
use caches on JarUrlConnection, which when closed would also close the jar file
for other resources using it, such as the AliasRegistry. This, combined with the
fact that the AliasRegistry could be created simultaneously by many threads/pipelines
could cause issues during AliasRegistry creation leading to failure to create a
pipeline.

* Avoid use of URLConnection caching when getting alias yaml resource
* Use idiomatic ruby when accessing Java getInstance method

Co-authored-by: Andrea Selva <[email protected]>
  • Loading branch information
robbavey and andsel authored Apr 26, 2022
1 parent 96f7e29 commit 4e77f1b
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 13 deletions.
2 changes: 1 addition & 1 deletion logstash-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ dependencies {
runtimeOnly 'commons-logging:commons-logging:1.2'
// also handle libraries relying on log4j 1.x to redirect their logs
runtimeOnly "org.apache.logging.log4j:log4j-1.2-api:${log4jVersion}"
implementation('org.reflections:reflections:0.9.11') {
implementation('org.reflections:reflections:0.9.12') {
exclude group: 'com.google.guava', module: 'guava'
}
implementation 'commons-codec:commons-codec:1.14'
Expand Down
2 changes: 1 addition & 1 deletion logstash-core/lib/logstash/plugins/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def initialize(alias_registry = nil)
@registry = java.util.concurrent.ConcurrentHashMap.new
@java_plugins = java.util.concurrent.ConcurrentHashMap.new
@hooks = HooksRegistry.new
@alias_registry = alias_registry || Java::org.logstash.plugins.AliasRegistry.new
@alias_registry = alias_registry || Java::org.logstash.plugins.AliasRegistry.instance
end

def setup!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.net.URLConnection;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.Collections;
Expand Down Expand Up @@ -123,7 +125,20 @@ Map<PluginCoordinate, String> loadAliasesDefinitions(Path yamlPath) {

Map<PluginCoordinate, String> loadAliasesDefinitions() {
final String filePath = "org/logstash/plugins/plugin_aliases.yml";
final InputStream in = AliasYamlLoader.class.getClassLoader().getResourceAsStream(filePath);
InputStream in = null;
try {
URL url = AliasYamlLoader.class.getClassLoader().getResource(filePath);
if (url != null) {
URLConnection connection = url.openConnection();
if (connection != null) {
connection.setUseCaches(false);
in = connection.getInputStream();
}
}
} catch (IOException e){
LOGGER.warn("Unable to read alias definition in jar resources: {}", filePath, e);
return Collections.emptyMap();
}
if (in == null) {
LOGGER.warn("Malformed yaml file in yml definition file in jar resources: {}", filePath);
return Collections.emptyMap();
Expand Down Expand Up @@ -177,7 +192,15 @@ private Map<PluginCoordinate, String> extractDefinitions(PluginType pluginType,
private final Map<PluginCoordinate, String> aliases = new HashMap<>();
private final Map<PluginCoordinate, String> reversedAliases = new HashMap<>();

public AliasRegistry() {
private static final AliasRegistry INSTANCE = new AliasRegistry();
public static AliasRegistry getInstance() {
return INSTANCE;
}

// The Default implementation of AliasRegistry.
// This needs to be a singleton as multiple threads accessing may cause the first thread to close the jar file
// leading to issues with subsequent threads loading the yaml file.
private AliasRegistry() {
final AliasYamlLoader loader = new AliasYamlLoader();
final Map<PluginCoordinate, String> defaultDefinitions = loader.loadAliasesDefinitions();
configurePluginAliases(defaultDefinitions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

package org.logstash.plugins.discovery;

import com.google.common.base.Predicate;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.logstash.plugins.AliasRegistry;
Expand Down Expand Up @@ -61,18 +60,17 @@ public final class PluginRegistry {
private final Map<String, Class<Codec>> codecs = new HashMap<>();
private static final Object LOCK = new Object();
private static volatile PluginRegistry INSTANCE;
private final AliasRegistry aliasRegistry;
private final AliasRegistry aliasRegistry = AliasRegistry.getInstance();

private PluginRegistry(AliasRegistry aliasRegistry) {
this.aliasRegistry = aliasRegistry;
private PluginRegistry() {
discoverPlugins();
}

public static PluginRegistry getInstance(AliasRegistry aliasRegistry) {
public static PluginRegistry getInstance() {
if (INSTANCE == null) {
synchronized (LOCK) {
if (INSTANCE == null) {
INSTANCE = new PluginRegistry(aliasRegistry);
INSTANCE = new PluginRegistry();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.logstash.instrument.metrics.AbstractMetricExt;
import org.logstash.instrument.metrics.AbstractNamespacedMetricExt;
import org.logstash.instrument.metrics.MetricKeys;
import org.logstash.plugins.AliasRegistry;
import org.logstash.plugins.ConfigVariableExpander;
import org.logstash.plugins.PluginLookup;
import org.logstash.plugins.discovery.PluginRegistry;
Expand Down Expand Up @@ -83,7 +82,7 @@ public static IRubyObject filterDelegator(final ThreadContext context,
}

public PluginFactoryExt(final Ruby runtime, final RubyClass metaClass) {
this(runtime, metaClass, new PluginLookup(PluginRegistry.getInstance(new AliasRegistry())));
this(runtime, metaClass, new PluginLookup(PluginRegistry.getInstance()));
}

PluginFactoryExt(final Ruby runtime, final RubyClass metaClass, PluginResolver pluginResolver) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class AliasRegistryTest {

@Test
public void testLoadAliasesFromYAML() {
final AliasRegistry sut = new AliasRegistry();
final AliasRegistry sut = AliasRegistry.getInstance();

assertEquals("aliased_input1 should be the alias for beats input",
"beats", sut.originalFromAlias(PluginType.INPUT, "aliased_input1"));
Expand Down

0 comments on commit 4e77f1b

Please sign in to comment.