search

feather-rs / feather

A Minecraft server implementation in Rust
Apache License 2.0
2.61k stars 141 forks source link

Feather Plugin System #309

Open ambeeeeee opened 4 years ago

ambeeeeee commented 4 years ago

Tracking Issue: Feather Plugin System

#106 is semi-outdated but its recommended to give it a glance, #308 should be used to discuss what kinds of plugins should be developed as part of the feather-rs project.

The idea of implementing a plugin system has been discussed more often recently, so its time to look at creating the plugin system more seriously. #106 did, however, have a good list of things that are important for the plugin system implementation. These will be mirrored here.

This list may be changed based on discussion, and it may be incomplete. The goal is to have plugins be able to do as much or nearly as much as code placed directly inside of feather can do, with as little overhead as possible. Obviously, as will be covered farther down, there will be overhead with the currently proposed methods.

Previously, there has been debate on whether to use dylibs or wasm for the plugin system, however as caelunshun says in a discord message, wasm is probably our best choice at this point due to its cross platform and sandboxing.

Feel free to add to the discussion (and suggest changes to the plan)! :)

Schuwi commented 4 years ago

I guess #213 should be linked here as well?

ambeeeeee commented 4 years ago

I guess #213 should be linked here as well?

That implementation looks like it was based on dynamic linking, the goal currently is wasm. (might be wrong I didn't really look at it too hard)

Defman commented 4 years ago

I think the goal is WASM in the long long run (when it gets the required feature set). For now, we are aiming for Dlibs and having a plugin database with pre-compiled plugins for the most common targets (windows, osx, Linux). This, however, would require some additional infrastructure and build server, maybe we can offload some of the building to GitHub, just maybe.

caelunshun commented 4 years ago

I think the goal is WASM in the long long run (when it gets the required feature set). For now, we are aiming for Dlibs and having a plugin database with pre-compiled plugins for the most common targets (windows, osx, Linux). This, however, would require some additional infrastructure and build server, maybe we can offload some of the building to GitHub, just maybe.

I'm starting to think that WASM will be adequate. I've done some looking into the WASM bytecode outputted by rustc and found that most of my problems with it have been solved.

The only thing WASM currently lacks is networking support, which is critical in the long run. The WASI spec has an issue open for it: https://github.com/bytecodealliance/wasmtime/issues/70. I hope that eventually (at least by the time Feather is ready for actual use, which will be a while) they'll settle on a solution there.

Dynamic linking would be nice, but it comes with a whole slew of problems... security/sandboxing are impossible, plugin reloading is difficult and platform-sensitive, and maintaining the build infrastructure will be a pain. I think that WASM is the best choice in the long run.

ambeeeeee commented 4 years ago

I've been working on some basic plugin stuff. My working branch can be found here.

So far I've got:

I'd like to note that the API design is in no way final, or even preliminary. I would like to discuss how we would like to lay it out, and there are still things to add with my current testing API. The API currently only serves the purpose of testing basic requirements.

I'd like some feedback from @caelunshun and anyone else who can provide it on the current way that I have plugins implemented, I think its kinda okay but I know @caelunshun mentioned registering systems from plugins. This may or may not be impossible due to ownership issues but we can certainly look into it.

As for version and name, I believe somewhere I saw a method that implied that you can store discrete data alongside a WASM module, so that would be useful. Otherwise there could always be a function like fn plugin_info() -> (Name, Version). (Don't pick that apart it's just an example and would have to be implemented very differently)

The source for the testing plugin is (basically)

extern "C" {
    fn print(ptr: *const u8, len: usize);
}

#[no_mangle]
pub extern fn on_load() {
    let hello = "Hello from a Plugin!";
    unsafe {
        print(hello.as_ptr(), hello.len());
    }
}

#[no_mangle]
pub extern fn on_tick() {
    let to_print = "Plugin just ticked, awesome!";

    unsafe {
        print(to_print.as_ptr(), to_print.len())
    }
}

Obviously we don't want consumers of the API to deal with interfacing with the API directly (since its ugly and can't use rust types). Alongside plugin API development there will also have to be a consumer API for the WASM plugins to make it easier and safer for them to interact with the API.

caelunshun commented 4 years ago

@amber Thanks, looks like a good start!

Some comments...

I also scrapped together something here to experiment with what the high-level wrapper API might look like. All the functions there are todo!()s, but we can implement them on top of the C headers easily enough.

I do feel it's best to hold off on implementing a plugin API at least until the 1.16 branch is more fleshed out. As so little functionality has been implemented, there could still be architectural changes over the next couple weeks. In the meantime, though, experimentation like this is definitely welcome! We need to test out different designs and see what works best :)

ambeeeeee commented 4 years ago

Thanks! I was planning on looking at integrating nicely with fecs! I'll take a look at your api and namespace (and underscore ;) ) builtins. Thanks!

ambeeeeee commented 4 years ago

Awesome! I've done some work on my implementation.

The other two changes are planned, I will probably work on switching crates tomorrow.

Current issues:

  1. Complex FFI interface, I'm not sure if there's a way to fix this. It might just be a fact of life, I can write wrapper types to make it safer for working with WASM inside feather. I've never worked with FFI before so I'm not even sure myself.
  2. Massive delay when starting the server, I swear this wasn't a problem before? (debug build)
  3. Potentially leaking memory, I'm not sure if anything actually gets freed in __quill_free

(My friend also pointed out that symbols beginning with two underscores are reserved in C, might wanna take that into account)

If you're curious on what plugins look like right now, here is a link to a hastebin. I can upload the plugin source to GitHub eventually, I need to get it to compile to WASM target without specifying in the build command first.

ambeeeeee commented 4 years ago

If you read this before 18:34Z, read it again. I've massively edited it.

I've implemented system registration in my fork. Additionally, I have made the testing plugin's source available on GitHub!

Unfortunately, I'm not entirely happy with the implementation of system registration from WASM :cry: It requires:

  1. All systems are registered with __quill_setup
  2. Systems are called using their exported symbol (check the plugin source if that not clear)
  3. I had to duplicate the current executor implementation to add another parameter to it (Plugin and `State)

Otherwise I like it, and it works!

Minor changes:

Another concern is the fact that WASM adds about 70 compilation units, I understand that there have been concerns about compile times in the past but it should be okay, especially if we leverage WASM to implement game logic since it would allow us to do extremely fast iteration.

@caelunshun I'd like some input from you, mainly: how should plugins integrate with the "event loop" of the server, and how should I handle needing two parameters for executing plugin systems. I tried using a tuple and a struct but I wasn't able to get them to work.

I have NOT switched to the wasmer crate yet, and that is my current task :).

caelunshun commented 4 years ago

@amber Thanks, this is looking good.

I noticed that you implemented plugin systems by having a single system invoke each plugin for each stage. I think it would be preferable to have plugin systems as first-class citizens, probably by changing the systems executor to store enum System { Native(fn(&mut State) -> SysResult), Plugin(PluginSystem) } or something of the like. The benefit if this is that we can order and instrument all systems regardless of whether they're defined in native code or in a plugin.

All systems are registered with __quill_setup

That seems good to me, since that's how the native codebase works as well (on the 1.16 branch). If you have concerns about that, please do share them.

I had to duplicate the current executor implementation to add another parameter to it (Plugin and `State)

It would make sense to me to intertwine the ecs and plugin crates, since we'll need special support in the ECS anyway to handle plugins. So there's no need to duplicate the executor; we can just make changes to the original one.

I created a crate that has the FFI types for the current API. This is to make code cleaner and to make using the interface between WASM and feather easier for me (and implementing new things :) ). The crate is currently in my testing plugin repo, since I decided that was the best place to put it.

Sounds good, that sort of high level interface is what we're looking at going into the future. I'll probably submit a bunch of commits or PRs on making this API as friendly as possible, as I have lots of ideas about what it should look like :)

Additionally, I cut down on how much unwrapping is in the plugin implementation, since unwrapping is no fun.

Yes, we don't want any unwrapping at all in the server, unless we're absolute certain the unwrap will never panic. Feather should never panic—that's another one of the improvements in the 1.16 branch (since systems now return results).

I'd like some input from you, mainly: how should plugins integrate with the "event loop" of the server

I'm not sure what you mean by "event loop," but currently I think it's adequate just to support plugins registering systems. (I haven't implemented event handling on the 1.16 branch yet—once I do, then we can look at how an event handler API might work.)

how should I handle needing two parameters for executing plugin systems

I'm not sure what these two parameters are, could you clarify that?

Overall, thanks for working on this! I'm glad to see we're getting some real implementation work going forward.

ambeeeeee commented 4 years ago

I sent stuff in Discord, but I think I'll lay it out here as well. With the upgrade to wasmer, there is less implicit passing of things to imported functions, thus I've had to implement that logic myself. My current logic uses 3 nightly features and 2 unsafe blocks. I haven't gotten around to adding some // SAFETY comments because I'd like to hear what you think before we decide to leave it in.

Click here to see what I did

I believe that it is entirely safe to do this, the WASI implementation in wasmer seems to do something similar. However, I wanted you opinion before rolling with it. In the future if we need to pass more state to imported functions, we can just pass a struct.

Thanks :)

ambeeeeee commented 4 years ago

We (@Defman, @Schuwi, @amberkowalski) have been discussing how plugin dependencies should work, and we've come up with a plan. As you've mentioned, plugins will be a tar that contains (at minimum) a plugin.toml and a [anything].wasm. The most basic form of a plugin.toml is as follows

[plugin]
name = "plugin_name"
pretty_name = "Fun plugin"
version = "0.1.0"

[dependencies]
feather = "0.6.0"

Implementation details (macros) have changed since we've put a few hours into trying to flesh out how we want to do this. However, the general idea is the same (and we're still using tomls).

All the versions in plugins use semver, of which the importance will be clear shortly. As you can see, at its most basic, the plugin.toml contains metadata for the plugin and what feather version it should be run with (uses semver so there can be more complex versioning requirements).

However, other plugins as dependencies are much more complex. This is because plugins are dealt with by end-users and for their convenience and safety, there needs to be additional checks to make sure feather doesn't crash or, even worse, invoke UB. Because of this, we've decided to add additional manifests/sections to a plugin's package/toml.

For example, take these two plugins.

The #[quill_export] macro indicates that the struct should be added to a plugin's exports, meaning that other plugins will at some point be able to access memory defined with the format of that struct. Thus, it is important to ensure that struct layouts are the exact same.

The #[quill_import("depname")] macro indicates that the plugin will be expecting an export from depname, so it is added to a plugin's [imports.depname]. Once again, this is important to ensure struct layouts are correct.

With the output of these two macros, feather can make sure that the struct layout on the dependency's exports is the same as what the plugin is expecting it to be, and thus can ensure type safety between plugins at runtime. This allows us to not strictly version every plugin, since as long as the interface is the same and semver hasn't indicated breaking changes, the plugins will still work together.

When adding a plugin dependency, it is added to the plugin.toml in the dependencies section. It is versioned with semver to make sure that breaking changes that don't necessarily change struct signatures are caught.

Here is an example of a plugin exporting a function. Keep in mind that the plugin user will NOT have to write the export manually, they will just use the macros. It is extremely likely exported and imported types will be moved to their own files (exports.toml and imports.toml respectively).

In the event that a plugin's imports do not match a dependency's exports, the plugin will fail to load, printing an error to the console.

caelunshun commented 4 years ago

Hi @amber,

Thank you, that looks good in general. I do have some feedback on where this is going in the future.

For the crate structure, I think the following design makes sense:

Also, I believe it's best to maintain a clear distinction between "Quill, the plugin API" and "Feather, the server that supports Quill plugins." Since this API could be implemented by a server written in any language with WASM bindings, it makes sense to keep the two entities separate. This also has the benefit that Feather semver and Quill semver are no longer intertwined. If we release a Feather version with new features, then we can keep plugins compatible by not incrementing the Quill version. This would have the consequence that feather should be changed to quill under the dependencies section in the above plugin manifest.

In terms of inter-plugin interaction—the only problem with your solution is that an upstream plugin can change a struct's layout without making a semver change, resulting in UB in a downstream plugin. This is a major security vulnerability. We need to put some more thought into how we ensure struct layouts are semver-correct.

caelunshun commented 4 years ago

Here's my draft of the high level API (no functions are actually implemented): https://docs.rs/quill-prototype/0.0.0-prototype.1/quill_prototype/ I'd love some feedback on how this looks, but I hope it will provide some guidance as to what the eventual API should look like.

I've started out very barebones. In particular, plugins can't add/access custom components at this time. However, I consider what's in the draft a good baseline API which we can build on in the future once we settle on a good design.

ambeeeeee commented 4 years ago

@caelunshun Lots of changes :D.

quill-internals

This is the new name for the crate which holds logic that should only be used by feather and the quill API implementation. Below is a list of changes I've made to it.

I would like it if we could move this crate into the feather group. Most likely under a quill repository, with this as a distinct crate within the repository. Additionally, I am requesting to be added as a maintainer for the repository this gets moved into (if it does) to help facilitate plugin API development.

quill-internals::module_externs

This is a crate meant to hold safe wrappers for standard quill functions, right now it only contains a super basic (and slightly incorrect, but of course still safe) implementation for log. As development continues it is likely we will define more required exports from an environment that supports the quill API. This module is not intended to be used directly by a plugin. The user-consumed plugin API will likely simply re-export its functions and required types.

feather

I've updated my branch implementing plugin support to use the new quill-internals library and types, as well as implementing the log function (base implementation, its not complete). The branch contains test-plugin compatible with the newest update. I have updated module naming to follow your "quill is just an API we're implementing" naming scheme that you mentioned you wanted. I have not changed anything else since we last spoke about the plugin system, however.

test-plugin

I've updated test-plugin to use quill-internals types, however with the wrapper functions and into() implementations, it doesn't use them much. Obviously we're never going to use the types in plugins directly, but this is okay for a testing environment.

I'm most likely going to be at work the whole time you're online unfortunately (until 8 PM PDT, 10 PM CDT). Please leave your comments here as you have been doing :smile:

ambeeeeee commented 4 years ago

Today's update, woo.

quill-internals

See here.

test-plugin

Updated to work with the latest quill-internals

feather

Added support for tracking allocations in a testing branch that hasn't been pushed, it outputs 

2020-09-23 16:35:20,066 DEBUG [feather_quill] New Alloc: Size: 14 Align: 1 Pointer: WasmPtr(0x110008)
2020-09-23 16:35:20,066 DEBUG [feather_quill] New Alloc: Size: 5 Align: 1 Pointer: WasmPtr(0x110020)
2020-09-23 16:35:20,066 DEBUG [feather_quill] New Alloc: Size: 11 Align: 1 Pointer: WasmPtr(0x110030)
2020-09-23 16:35:20,066 DEBUG [feather_quill] New Alloc: Size: 28 Align: 4 Pointer: WasmPtr(0x110040)
2020-09-23 16:35:20,066 DEBUG [feather_quill] New Alloc: Size: 80 Align: 4 Pointer: WasmPtr(0x110060)
2020-09-23 16:35:20,066 DEBUG [feather_quill::manager] Registered system test_system for Some("Testing Plugin") [Some("1.0.0")]
2020-09-23 16:35:20,066 DEBUG [feather_quill] Dealloc: Size: 14 Align: 1 Pointer: WasmPtr(0x110008)
2020-09-23 16:35:20,066 DEBUG [feather_quill] Dealloc: Size: 5 Align: 1 Pointer: WasmPtr(0x110020)
2020-09-23 16:35:20,066 DEBUG [feather_quill] Dealloc: Size: 11 Align: 1 Pointer: WasmPtr(0x110030)
2020-09-23 16:35:20,066 DEBUG [feather_quill] Dealloc: Size: 28 Align: 4 Pointer: WasmPtr(0x110040)
2020-09-23 16:35:20,066 DEBUG [feather_quill] Dealloc: Size: 80 Align: 4 Pointer: WasmPtr(0x110060)

It turns out my FFI layer was prone to both UB and memory leaks, isn't that wonderful? I was deallocating at the right spots and with the correct sizes (in most cases), but the alignment is off. The rust docs say that passing layouts to the allocator for dealloc that are in any way different than the ones used to allocate is UB, and thus I was creating UB. I rewrote the entire FFI layer that uses allocations to store the size and align of the types (32 bit) so I could properly deallocate them. The result of these efforts is seen above. The updates for this will be pushed either tonight or tomorrow, I have to clean them up.

If you're online today I'm probably going to be at work (until 8 PM PDT, 10 PM CDT). Please leave your comments here as you have been doing. (Make sure to read yesterday's)

caelunshun commented 4 years ago

Hi @amber,

I'm so sorry about my absence these past few days. That's totally on me, and I'll make sure I'm available over the next few weeks.

I'll be on Discord for the rest of tonight and throughout the weekend, so if you want to discuss anything further I'm ready :)

I'd like to offer the crate as MIT or Apache 2.0, however I noticed feather is just Apache 2.0. Please let me know if you're okay with me licensing quill-internals under both licenses.

It sounds reasonable to me to license quill under the standard Rust library license, just so we have GPL 2.0 compatibility. So this is fine by me.

Additionally there were concerns about de-generalizing the prefix from FFI to Plugin, this is valid but there is no need for general prefixes since this library is only for implementing the quill plugin api.

I think it even makes more sense to use the Plugin prefix, since it makes it explicit that the type is used for interop with plugins.

I would like it if we could move this crate into the feather group. Most likely under a quill repository, with this as a distinct crate within the repository. Additionally, I am requesting to be added as a maintainer for the repository this gets moved into (if it does) to help facilitate plugin API development.

Sure thing, I'll create a quill repository in the feather-rs organization and give you access.

One thing—for consistency with Rust naming conventions, I think quill-sys would be a better name for Quill, as it's a raw FFI API. quill-internals feels more ambiguous to me.

The tracking allocations look good, those should be useful for debugging.

Thanks for all your work on this, and again I apologize for my absence :)

Bobbyjoness commented 3 years ago

I see that you guys are considering using wasm. I honestly don't even understand why you would twist yourself into using that. Why not use Lua? It is a very common language for plugins.

ambeeeeee commented 3 years ago

I see that you guys are considering using wasm. I honestly don't even understand why you would twist yourself into using that. Why not use Lua? It is a very common language for plugins.

Performance and power. WASM is way more powerful and allows us to use a sane language to develope plugins, and additionally allows any language that can compile to WASM to be used.

Its also probably faster, idk.

Bobbyjoness commented 3 years ago

Luajit is blazingly fast. Probably faster than any current wasm interpreter. Next I do not believe being indecisive about the plugin programming language is a good idea. It has the potential to hinder the plugin community. Lua is a good fast language that a lot of people are familiar with. Simple plugins will be a lot quicker to write in Lua than in rust. Lua is safe when embedded in rust. It is much better choice imo.

On Sun, Nov 8, 2020, 12:07 AM Amber Kowalski notifications@github.com wrote:

I see that you guys are considering using wasm. I honestly don't even understand why you would twist yourself into using that. Why not use Lua? It is a very common language for plugins.

Performance and power. WASM is way more powerful and allows us to use a sane language to develope plugins, and additionally allows any language that can compile to WASM to be used.

Its also probably faster, idk.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/feather-rs/feather/issues/309#issuecomment-723531261, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6IUHQU3TKHTYUTF6EOQD3SOYRRZANCNFSM4RKLZ56A .

ambeeeeee commented 3 years ago

Probably faster than any current wasm interpreter.

Our WASM is JIT'd.

Next I do not believe being indecisive about the plugin programming language is a good idea. It has the potential to hinder the plugin community.

We have no plans to offer first party support for anything other than Rust, but that is mentioned as a benefit because it technically is.

Simple plugins will be a lot quicker to write in Lua than in rust.

Potentially, but a large amount of plugins are not simple, and writing them in lua would bu more complex than in Rust.

Lua is safe when embedded in rust.

So is WASM, except WASM has the potential to be even safer due to the simple base architecture. Whether this is true in practice, at this point, is debatable.
The lua libraries are not guaranteed to be safe either (even by the devs themselves).

hazae41 commented 3 years ago

How about Deno and plugins written in TypeScript/WASM ? This would solve the WASI problem since feather would still be written in native Rust. And this would be secure as hell since TypeScript/WASM plugins would be sandboxed.

There are multiple ways:

Using custom Minecraft ops we could make TypeScript plugins very easily, and with an ecosystem like Saurus, which uses git submodules, this would be awesome.

Also, Deno develops a WASI interface https://deno.land/std@0.77.0/wasi, so feather plugins would be able to do native things, sandboxed.

ShadowJonathan commented 3 years ago

Heya, i'm just flying by here from that wasmtime networking issue mention, I find the project in general pretty interesting (a minecraft server in rust? hell yeah), and the initiative here as well.

A few questions, though;

A more technical question; Is it possible to "expose" a function into the wasm runtime that can then be called upon? Maybe this could be used in a syscall-esc fashion, with the internal feather plugin API crate using it to call back to feather (and the like)