Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deno.openPlugin resolution is not inline with the rest of Deno #3448

Closed
lucacasonato opened this issue Dec 6, 2019 · 16 comments
Closed

Deno.openPlugin resolution is not inline with the rest of Deno #3448

lucacasonato opened this issue Dec 6, 2019 · 16 comments
Labels
suggestion suggestions for new features (yet to be agreed)

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Dec 6, 2019

Just played around with the new modules and found some inconsistencies with the rest of Deno.

  • can not resolve file://${path} paths (resolves to ${cwd}/file:/${path})
  • can not resolve external paths (http:// and https://)
  • relative paths resolve relative to ${cwd} and not relative to the module path

The resolution algorithm that is used by import should probably also be used for plugins.

@nayeemrmn
Copy link
Collaborator

It's not a JS module, module resolution is its own thing. While URL plugins are interesting 😄, I think it's properly consistent with other FS ops.

I don't see why those shouldn't support the file: protocol, though.

@lucacasonato
Copy link
Member Author

It is indeed not a JS module, but it is a module in the sense that you can use it to extend the funtionality of your programm - sort of like WASM. Also I don't think its really comparable to an fs op because its purpose is enirely different. It should fall inline with how imports work because thats what openPlugin does - it imports a plugin for use.

I think it is important that URL plugins are a possibility, because otherwise it is very hard to disitribute them. The current way to make it work would be to manually fetch the plugin, store it in a temp dir - where we don't control the caching - and then pass the temp path into openPlugin. Very tedious. We solved this already with JS, TS, WASM, ect. so I would argue that the same should apply to plugins.

On that note, if URL plugins are going to be a thing we might want to make openPlugin async and add a openPluginSync method so we dont have to block the main thread while downloading and caching a plugin.

@nayeemrmn
Copy link
Collaborator

It is indeed not a JS module, but it is a module in the sense that you can use it to extend the funtionality of your programm

And that sense isn't sufficiently close to language level. Can you think of anything which works relative to module location that doesn't contain the import keyword? I wouldn't want to start confusing this distinction. It's normal for scripting environments to do everything with respect to CWD - the language's core module system is usually the exception.

When you talk about it's purpose being similar to modules, that's in a really high level sense. What we're doing is reading a data file and creating some functions from it.

@lucacasonato
Copy link
Member Author

lucacasonato commented Dec 6, 2019

I get what you mean. If openPlugin stays relative to cwd then that is fine I guess. You can always generate the relative path yourself with import.meta.url. Later we might want to allow you to directly import plugins via the import statement like we do for WASM - this would then be relative to your module path rather than cwd - like the WASM implementation we have.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 6, 2019

I don't see why we wouldn't use the same logic to resolve plugins as we would other resources. Supporting remote plugins though (via HTTP) I think is potentially a different thing. Even though they are different, having two different resolution schemes is fairly non-sensical. The only reason for it would be to confuse users.

@afinch7
Copy link
Contributor

afinch7 commented Dec 11, 2019

Resolving and loading plugins like modules would be nice, but there are some difficulties here. The underlying c function dlopen that supports the entire plugin functionality can only accept file paths.

I have considered emulating the underlying function of dlopen(load file contents to ram, inform kernel this memory space is executable, dynamically link resources from the memory namespace, etc). This might be a valid option ,but would add a significant amount of complexity and would require specific support on our end for each kernel.

Maybe a simpler approach here is to cache the plugin file like we do compiled JavaScript. Either way downloading a file and loading directly into memory as executable presents a lot of security concerns.

A potential third approach of adding custom compiler support and building a custom cargo/rust compiler might be a good solution here as well. I like this solution the most, since a custom compiler could actually generate TypeScript bindings to go on top of said plugins. This also avoids downloading a compiled binary and loading directly to memory. When I first started working on plugins I thought that it would be cool if we could just import a cargo.toml file and compile like we do TypeScript, but I quickly realized how complex that would be(especially for http/https imports). A solution like that would have been almost as much complexity as the rest of deno at the time for a feature that isn't even the main function of deno. It makes a lot more sense for something like this to be it's own project.

The existing plugin system is intended to be very simple it's primary goal is to enable calling custom native code from the deno runtime. It's not a framework. Just a simple way to call into custom rust code. While I would like to be able to resolve plugins like imports it's not possible to determine the caller from the rust side like a referrer in a import resolution. For now I think the best we can really expect is some tooling in std to make this a little easier and maybe some tweaks to resolution to allow absolute full paths(file:///some/path). I have found using import.meta.url plus join to be a pretty good solution right now. I have some working code using what I would consider standard practices in the form of deno_in_deno(check out the use_new_plugin_api branch for more up to date examples).

@lucacasonato
Copy link
Member Author

I think we should go for the approach where we download into a folder in DENO_DIR and then also load them from there. I think manually loading the contents into memory, marking it as exec and directly calling the dlopen syscall is too complicated.

If we build remote resolution of plugins right into the 'secure' part of Deno then this makes the experience of using plugins simpler for users. We can use the same caching algorithms as JS imports, we can respect the --no-remote flag, we can respect the --reload flag, and all of the cached deps (js, ts, wasm and plugins) can all be stored in the same dir - DENO_DIR. Implementing this in std would just require us to expose all of these flags to the user via ops. It seems nicer to just use the existing rust code. We might even be able to integrate this into the lock file which would be very nice.

I don't think there are any more security implications in running with --allow-plugin than there are using --allow-run. --allow-run can also execute arbitrary binaries. Specifying either of these permissions gives you full system access and all of the Deno security promises are gone. I understand loading a dynamic lib is more scary as you are right inside of the Deno process, but ey, that's just one of the tradeoffs you have with plugins - all security is gone.

@nayeemrmn
Copy link
Collaborator

I was against applying module semantics when plugins are brought in with a function call.

Since this is maybe going in that direction, I'm perfectly okay with treating them as modules if they are actually brought in using the import syntax: import { some_op as someOp } from "./plugins/libsome_op.so" assuming there are no other issues with that.

@afinch7
Copy link
Contributor

afinch7 commented Dec 14, 2019

We really don't even have a good way to resolve relative paths in function calls based on the "referrer" like we can with imports anyways, so current working directory is really the best we can do for now. Also the filename and extension for a plugin will vary for each os, so static imports are not really an option. I.E. libplugin.so on linux becomes plugin.dll on windows and libplugin.dylib on macos.

@nayeemrmn
Copy link
Collaborator

Also the filename and extension for a plugin will vary for each os, so static imports are not really an option. I.E. libplugin.so on linux becomes plugin.dll on windows and libplugin.dylib on macos.

Well you'd obviously resort to dynamic imports if you want compatibility :). Static imports are just that - an option - if you don't need this. If it fails you can throw a ModuleResolution error with a message saying exactly what the problem is.

^ This does sound like a way of introducing some bad practices. I'm also completely fine with the way it is now. That means no use of the import keyword therefore no module-like semantics. Even if it was easy.

@ry
Copy link
Member

ry commented Dec 14, 2019

@lucacasonato that is the eventual goal. In fact the original patches from @afinch7 were doing something along those lines IIRC. However we need some foundational infrastructure - having basic dlopen functionality is important and close to the OS. We will add sugar on top of that.

@manyuanrong
Copy link
Contributor

I am currently trying to write a mongodb driver with plugin, and I have some opinions on this.

  1. The current use of openPlugin makes it very difficult to distribute our plugins. This will be very difficult in practical applications.
  2. If our ultimate goal is to make the plugin loading consistent with js / ts, using import loading, I think it is good. But as discussed earlier, the file name is different for different platforms, which is a challenge. Currently I can think of using dynamic import, but this causes difficulties in practical applications, because usually a local plug-in will be very large, dynamic import will cause the first run to be slow, and it will make prefetch difficult.

@buckle2000
Copy link
Contributor

I don't think is import is good for this. Different platforms use different plugins, so dynamic import makes more sense.

Also, I support making openPlugin accepts http URL and caching plugins to $DENO_DIR/plugins.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 6, 2021
@lucacasonato lucacasonato added the suggestion suggestions for new features (yet to be agreed) label Jan 13, 2021
@stale stale bot removed the stale label Jan 13, 2021
@bartlomieju
Copy link
Member

Plugins are getting removed in 1.13 (#8490) so this issue is no longer relevant.

@lucacasonato
Copy link
Member Author

lucacasonato commented Nov 30, 2021

In spirit this is still relevant. See #12943 for the follow up for FFI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

8 participants