Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

geyser: allow custom name in config file #33550

Merged
merged 7 commits into from
Dec 5, 2023
Merged

geyser: allow custom name in config file #33550

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
979ea2a
wrap dyn GeyserPlugin
fanatid Oct 5, 2023
b46199b
allow custom name from config
fanatid Oct 5, 2023
69281f8
use String instead of &str
fanatid Oct 5, 2023
f5bb21a
rename libname to name
fanatid Oct 11, 2023
ccb6061
Merge branch 'master' into geyser-multiple-plugins
fanatid Nov 10, 2023
ecdaff6
Merge remote-tracking branch 'upstream/master' into geyser-multiple-p…
fanatid Dec 5, 2023
d9932ea
fix setup_logger_for_plugin
fanatid Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions geyser-plugin-manager/src/geyser_plugin_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,48 @@ use {
libloading::Library,
log::*,
solana_geyser_plugin_interface::geyser_plugin_interface::GeyserPlugin,
std::path::Path,
std::{
ops::{Deref, DerefMut},
path::Path,
},
};

#[derive(Debug)]
pub struct LoadedGeyserPlugin {
name: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe libname? I do not see how this utilized to solve the problem. We intentionally request the plugin has different names so that load/unload can work reliably. Why cannot you give different names to different plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cannot you give different names to different plugins?

because for a new name you need to re-compile the whole plugin with hardcoded name in the code 🙂

solana/geyser-plugin-interface/src/geyser_plugin_interface.rs

Line 290 in 666ce9b

fn name(&self) -> &'static str;

Copy link
Contributor

@lijunwangs lijunwangs Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this problem can be solved by the plugin itself without the framework's involvement. The &static reference makes it a little tricky but not insurmountable. Here is some sample code to the effect:

use std::fs;
use std::sync::{Mutex, Once};

use std::collections::HashMap;

struct Plugin {
    name: Mutex<HashMap<i32, &'static str>>,
    initialized: Once,
}

impl Plugin {
    pub fn new() -> Self {
        Self {
            name: Mutex::new(HashMap::default()),
            initialized: Once::new(),
        }
    }

    pub fn load(&mut self, file: String) {
        self.initialized.call_once(|| {
            let file_content: String = fs::read_to_string(file).unwrap();
            let s = Box::new(file_content);
            let s = Box::leak(s); // it leaks once and only once, so still safe!
            self.name.lock().unwrap().insert(1, s);
        })
    }

    fn name(&self) -> &'static str {
        let data = self.name.lock().unwrap();
        let data = data.get(&1).unwrap();
        data
    }
}

fn main() {
    let mut s = Plugin::new();
    s.load("config.txt".to_string());
    println!("This is my content {}", s.name());
}

This way, you don't have to hard code the plugin name in the code and load it from the config file yourself.

Copy link
Contributor Author

@fanatid fanatid Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, nice workaround
I think name() happens before load() so call_once should be moved in this case

while this code probably would work I still think that supporting libname through config would be nice
for plugins users, it should be much easier to add one line to config instead of patching the plugin with the proposed code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, confirmed
on_load is called after name, so we can not take the name from the config file in the plugin

solana/geyser-plugin-manager/src/geyser_plugin_manager.rs

Lines 95 to 113 in 64b3613

if self
.plugins
.iter()
.any(|plugin| plugin.name().eq(new_plugin.name()))
{
return Err(jsonrpc_core::Error {
code: ErrorCode::InvalidRequest,
message: format!(
"There already exists a plugin named {} loaded. Did not load requested plugin",
new_plugin.name()
),
data: None,
});
}
// Call on_load and push plugin
new_plugin
.on_load(new_config_file)
.map_err(|on_load_err| jsonrpc_core::Error {

plugin: Box<dyn GeyserPlugin>,
}

impl LoadedGeyserPlugin {
pub fn new(plugin: Box<dyn GeyserPlugin>, name: Option<String>) -> Self {
Self {
name: name.unwrap_or_else(|| plugin.name().to_owned()),
plugin,
}
}

pub fn name(&self) -> &str {
&self.name
}
}

impl Deref for LoadedGeyserPlugin {
type Target = Box<dyn GeyserPlugin>;

fn deref(&self) -> &Self::Target {
&self.plugin
}
}

impl DerefMut for LoadedGeyserPlugin {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.plugin
}
}

#[derive(Default, Debug)]
pub struct GeyserPluginManager {
pub plugins: Vec<Box<dyn GeyserPlugin>>,
pub plugins: Vec<LoadedGeyserPlugin>,
libs: Vec<Library>,
}

Expand Down Expand Up @@ -264,7 +300,7 @@ pub enum GeyserPluginManagerError {
#[cfg(not(test))]
pub(crate) fn load_plugin_from_config(
geyser_plugin_config_file: &Path,
) -> Result<(Box<dyn GeyserPlugin>, Library, &str), GeyserPluginManagerError> {
) -> Result<(LoadedGeyserPlugin, Library, &str), GeyserPluginManagerError> {
use std::{fs::File, io::Read, path::PathBuf};
type PluginConstructor = unsafe fn() -> *mut dyn GeyserPlugin;
use libloading::Symbol;
Expand Down Expand Up @@ -307,6 +343,8 @@ pub(crate) fn load_plugin_from_config(
libpath = config_dir.join(libpath);
}

let libname = result["libname"].as_str().map(|s| s.to_owned());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make it optional now. If it is set in the config file, use it, otherwise old behavior to keep it backward compatible. I would like to still call it "name" instead of libname, we do not care about the libname. The lib path has the libname anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed libname to name. Value is already optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lijunwangs ping :)


let config_file = geyser_plugin_config_file
.as_os_str()
.to_str()
Expand All @@ -321,7 +359,7 @@ pub(crate) fn load_plugin_from_config(
let plugin_raw = constructor();
(Box::from_raw(plugin_raw), lib)
};
Ok((plugin, lib, config_file))
Ok((LoadedGeyserPlugin::new(plugin, libname), lib, config_file))
}

#[cfg(test)]
Expand All @@ -337,7 +375,7 @@ const TESTPLUGIN2_CONFIG: &str = "TESTPLUGIN2_CONFIG";
#[cfg(test)]
pub(crate) fn load_plugin_from_config(
geyser_plugin_config_file: &Path,
) -> Result<(Box<dyn GeyserPlugin>, Library, &str), GeyserPluginManagerError> {
) -> Result<(LoadedGeyserPlugin, Library, &str), GeyserPluginManagerError> {
if geyser_plugin_config_file.ends_with(TESTPLUGIN_CONFIG) {
Ok(tests::dummy_plugin_and_library(
tests::TestPlugin,
Expand All @@ -359,7 +397,7 @@ pub(crate) fn load_plugin_from_config(
mod tests {
use {
crate::geyser_plugin_manager::{
GeyserPluginManager, TESTPLUGIN2_CONFIG, TESTPLUGIN_CONFIG,
GeyserPluginManager, LoadedGeyserPlugin, TESTPLUGIN2_CONFIG, TESTPLUGIN_CONFIG,
},
libloading::Library,
solana_geyser_plugin_interface::geyser_plugin_interface::GeyserPlugin,
Expand All @@ -369,9 +407,9 @@ mod tests {
pub(super) fn dummy_plugin_and_library<P: GeyserPlugin>(
plugin: P,
config_path: &'static str,
) -> (Box<dyn GeyserPlugin>, Library, &'static str) {
) -> (LoadedGeyserPlugin, Library, &'static str) {
(
Box::new(plugin),
LoadedGeyserPlugin::new(Box::new(plugin), None),
Library::from(libloading::os::unix::Library::this()),
config_path,
)
Expand Down