Make AliasRegistry a singleton #14002
Conversation
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 elastic#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: elastic#13996, elastic#13666
|
Hi @robbavey I put some head on how to verify this, without anything tangible, but happy to be your reviewer. |
… in AliasRegistry creation fixed by PR elastic#14002
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.
|
@andsel I reproduced the issue by introducing a random delay ( Digging a little further, this would always happen just after the reflections library completed: Digging a little further, the root cause of this appears to a multi-threading issue in the reflections library used in the PluginRegistry (which is why your test program would not trigger the issue), where its use of caches, and in I've made changes to the PR -
These changes should fix the issue regardless of whether we use a singleton AliasRegistry, but I'm not sure why we would ever want one more than one AliasRegistry to be created anyway, so I'm inclined to keep that change anyway. WDYT? |
|
I absolutely agree to keep the
I wonder if we have other cases of libraries that touches jars and get this problem with the cached resource. The only other part where Logstash loads jars that comes to my mind, are the JDBC plugins. But in that case the classes are loaded leveraging JRuby so I think it's well behaving respect to this problem. |
|
@andsel Cool, this is ready for review. Trying to unit test this is somewhat tricky - I could only trigger this reliably by introducing an artificial randomized delay in the |
I agree with you, it's useless to put test code in production code. If it used some other interfaces that we could mock and reimplement in the test code to make it trigger, would have been nice to test, but since we don't use such a think it's fine for me to do not test it. |
Co-authored-by: Andrea Selva <selva.andre@gmail.com>
|
@logstashmachine backport 8.2 |
* 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 <selva.andre@gmail.com> (cherry picked from commit 4e77f1b)
* 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 <selva.andre@gmail.com> (cherry picked from commit 4e77f1b) Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
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