Error handling for plugin initialization method

[George3d6]

Plugins should never crash the agent if initialization doesn't work, instead it should just log and error, same goes for the receptor.

[Deedasmi]

21 forces agent plugin crates to return a Result(T: AgentPlugin, U: Display>. That type doesn't actually exist because it's not valid Rust, but that's basically how this works.

The downside is that the error messages are garbage. If any one plugin does not turn a result, the compiler error looks something like:

https://pastebin.com/yyyURJm1

We can make these error messages better, but it requires us to know the actual types that the plugin is returning. We can accomplish this by exporting a type from the plugins, similar to https://github.com/Deedasmi/Inquisitor/blob/catch_unwind2/agent/agent_plugins/alive/src/lib.rs#L11. We can export a similar type for errors, but ALL plugins would have to do this.

[Deedasmi]

Also, to clarify, we cannot stop plugins from crashing the agent entirely. A plugin could simply call std::process:exit(1). There is no way to stop this, not even panic::catch_unwind would prevent it. What we can do is assume plugins are well written/maintained and use the version selection feature of cargo to allow users to downgrade broken plugins/recompile without them. Good error management is still important, and what I'm going to be focusing on for this issue.

[George3d6]

Ah, of course, I don't mean plugins shouldn't be able to crash the agent, but any given plugin we include in the default build should have error handling in such a way as to not crash it.

If 3rd part plugins are introduced they should be either validated & tested (as should be the current ones) or simply not pre-packaged with the agent.

[George3d6]

I think having the plugin type define doesn't really help much. We could have better error messages by simply returning a "custom" errors in place where plugins are likely not to work. Ala what you did in this PR:

https://github.com/George3d6/Inquisitor/pull/21

By just having new return Result<Plugin, String> with a custom error (Eg. config not found, config incorrect). config itself could return a Result and new will just have to pass on the custom error.

[Deedasmi]

So #21 is the PR that returns those bad compiler errors if plugin authors don't follow the not-yet-created plugin creation guide. I do think this is acceptable under RTFM.

The thing about this PR, and why I would like to go this route, is I'm not requiring authors to return a Result<Plugin, String>. They can return any type that implements AgentPlugin, and any error that implements Display. The benefit is that they could return any type of error they wanted, as long as it was printable. You can change file_checker::new() into returning a std::io::Error if you wanted. Here is a quick example:

pub fn new() -> Result<Plugin, std::io::Error> {
    let mut new_plugin = Plugin {
        enabled: false,
        last_call_ts: 0,
        periodicity: 0,
        file_info_map: HashMap::new(),
    };
    Plugin::config(&mut new_plugin);
    if new_plugin.enabled {
        Ok(new_plugin)
    } else {
        Err(std::io::Error::from(std::io::ErrorKind::NotFound))
    }
}

Alive successfully loaded Command runner disabled entity not found

You could also return a failure::Error if you wanted to be cool like that. I feel like this makes things more flexible, since all we really want to do is log their error anyway.

[Deedasmi]

With #21 merged, the other thing this issue touches is the ready function. I could see why ready might panic (checks a file handle that existed on initialization but no longer exists). But I also think it's reasonable to expect that just means 'false'.

The benefit of making this return a result is that we can basically take a ready() -> Err as a 'stop checking this plugin' and drop it from our plugin array. It could also mean 'Let the receptor know that this plugin is failing'. Is that something we want?

[George3d6]

Hmh... ready() isn't really meant to let the receptor know the plugin is failing, if there's an issue plugins should return the error in gather(), giving read() an error logging possibility as well may be overkill. So if, say, a necessary file is "gone" ready should, at most, tell the plugin is ready and gather should send the error.

[George3d6]

Using read() to stop checking the plugin (because it's disabled) could work, but if a plugin is disabled checking that is basically just waiting for it to return that bool, which is basically an instant thing... so I don't think it would really be helpful in any realistic way performance wise.

[Deedasmi]

So there is nothing in ready to protect right? So this issue is done since you made to receptor catch new()?

Richard Petrie Kansas State University

Sent from a mobile device

On Mar 30, 2018 7:07 PM, George notifications@github.com wrote:

Hmh... ready() isn't really meant to let the receptor know the plugin is failing, if there's an issue plugins should return the error in gather(), giving read() an error logging possibility as well may be overkill. So if, say, a necessary file is "gone" ready should, at most, tell the plugin is ready and gather should send the error.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/George3d6/Inquisitor/issues/19#issuecomment-377649742, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE23rfzi69-OHueR8dI_OKXH3Q9ckRnJks5tjsjSgaJpZM4TB6zf.

[George3d6]

Hmh... I now see that I was the one to say ready should return an error... my bad, dunno what I was thinking when I wrote the issue :S

[George3d6]

I think the issue is done, yes.

[George3d6]

No, sorry, my bad, the receptor still doesn't catch new.

[Deedasmi]

https://github.com/George3d6/Inquisitor/blob/master/receptor/plugins/src/plugin_initialization.rs#L8

Looks like it does?