Feature: Native plugins

[afinch7]

See denoland/std#475 for WIP example usage.

[afinch7]

I might need some help with adding the cargo and libloading crates to third_party still having a few build errors with some dependencies.

[afinch7]

It's about 90% there at this point just need to fix some bugs, and figure out what to do with the new dependencies I added. Major TODOs still left:

• Solve dependency build issues or find a way to remove some dependencies
• Fix compiler getting stuck on module name I.E. Cargo.toml:11:30 - error TS8010: 'types' can only be used in a .ts file.
• Implement dispatch for custom ops
• Fill BindingDispatchContext with useful context functions and implement them
• Add a bunch of new tests for this
• Add permissions for loading native bindings

[afinch7]

Mostly finished at this point. Ended up dropping cargo integration, since it adds so much complexity. I might revisit cargo integration later. I also haven't added support for native binding loading via url.
The current usage is just to import the compiled dylib:

import { someOp } from "./binding_project/target/release/libbinding_project.so";

This poses some problems for cross platform mainly that the built artifact name varies from platform to platform. I didn't want to introduce any magic I.E. ./binding_project/target/release/binding_project -> ./binding_project/target/release/libbinding_project.so on linux.
I considered maybe a special extension to make it more obvious where this magic comes into play I.E. ./binding_project/target/release/binding_project.binding, but that also seems a little too magic as well.
What do you think @ry?

I also might need some help figuring out the current issue on appveyor or finding a simpler way to build my test binding plugin. I'm currently waiting on my changes to dlopen_derive to go through, so I don't have to setup builds for multiple versions of quote and syn.

[ry]

This is an awesome patch. Amazing work.

Adding a native bindings API needs to be considered very carefully. It has impacts on security (all bets are off when you execute some random native code), performance, and usability (the GYP situation in Node is one of my largest blunders).

I am not ready to commit to importing .so files via the module system. For one thing, this breaks browser compatibility; not such a big deal itself. It introduces need for declare_binding_plugin!. I wouldn't rule this out in the future, but I think for this first pass we shouldn't try to integrate dlopen into the existing module system.

There's also the question of how these .so files be built. It would be great to be able to import a Cargo.toml (as you had earlier) but this means we depend on some external tooling. (Depending on cargo is probably fine, but it needs to be considered as it would break the promise of Deno being completely dependency free.) I'd rather punt on the topic of integrating with a build system.

I propose having a dlopen op, and another op to call a function in the DLL (dlcall?). I think it's fine if we just support calling functions that have a specific function signature. I'd leave it up to you to how you think the most minimal function signature is. Something like typedef void verysimple(size_t len, void* data); would work for this first PR - but maybe that's too simple to get the example working. I think another good choice would be DispatchFn in core/isolate.rs:

deno/core/isolate.rs

Line 53 in 856c442

type DispatchFn = Fn(&[u8], Option<PinnedBuf>) -> Op;

I think making these changes will reduce the surface area of this proposal and allow us to land something basic before committing to defining how everything works upfront.

I suggest renaming --allow-native-bindings to --allow-dlopen.

[afinch7]

Great feedback. I needed to get out of my one track mindset a little here. Since I already decided that direct cargo import is a little too complicated for this one, I don't really see any reason it has to be a dylib that loads typescript instead of the other way around.
Thoughts on this TypeScript api?

export interface DynamicLibFn {
  dispatchSync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Uint8Array;

  dispatchAsync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Promise<Uint8Array>;
}

// A loaded dynamic lib function.
// Loaded functions will need to loaded and addressed by unique identifiers
// for performance, since loading a function from a library for every call 
// would likely be the limiting factor for many use cases.
/** @internal */
class DynamicLibFnImpl implements DynamicLibFn {
  
  private readonly dlFnId: number;

  constructor(dlId: number, name: string) {
    this.dlFnId = loadDlFn(dlId, name);
  }

  dispatchSync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Uint8Array {
    // Like the prior Deno.nativeBindings.sendSync
    return callDlFnSync(this.dlFnId, data, zeroCopy);
  }

  async dispatchAsync(
    data: Uint8Array,
    zeroCopy: undefined | ArrayBufferView,
  ): Promise<Uint8Array> {
    // Like the prior Deno.nativeBindings.sendSync but async
    return callDlFnAsync(this.dlFnId, data, zeroCopy);
  }

}

export interface DynamicLib {
  loadFn(name: string): DynamicLibFn;
}

// A loaded dynamic lib.
// Dynamic libraries need to remain loaded into memory on the rust side
// ,and then be addressed by their unique identifier to avoid loading
// the same library multiple times.
export class DynamicLibImpl implements DynamicLib {

  // unique resource identifier for the loaded dynamic lib rust side
  private readonly dlId: number;
  private readonly fnMap: Map<string, DynamicLibFn> = new Map();

  constructor(private readonly libraryPath: string) {
    // call some op to load the library and get a resource identifier
    this.dlId = loadDl(libraryPath);
  }

  loadFn(name: string): DynamicLibFn {
    // call some op to load the fn from this library and get resource identifier
    const cachedFn = this.fnMap.get(name);
    if (cachedFn) {
      return cachedFn;
    } else {
      const dlFn = new DynamicLibFnImpl(this.dlId, name);
      this.fnMap.set(name, dlFn);
      return dlFn;
    }
  }

}

The implementation for the test plugin/binding being something like this

const { platformFilename, DynamicLibary } = Deno.dlopen;

const dLib = new DynamicLibary("target/release/" + platformFilename("test_binding_lib"));
const testOpFn = dLib.loadFn("test_op");

export interface TestOptions {
    name: string;
}

export interface TestResponse {
    data: Uint8Array;
}

const textEncoder = new TextEncoder();

function encodeTestOp(args: TestOptions): Uint8Array {
    return textEncoder.encode(JSON.stringify(args));
}

const textDecoder = new TextDecoder();

function decodeTestOp(data: Uint8Array): any {
    return textDecoder.decode(data);
}

export const testOp = (args: TestOptions): any => {
    return decodeTestOp(
        testOpFn.dispatchSync(
            encodeTestOp(args),
        ),
    );
}

[afinch7]

@ry I made some major simplifications, and removed the import functionality PTAL.

[ry]

I don't like that this breaks my usual ./tools/build.py && ./tools/test.py

Let me see if I can whip up a BUILD.gn to do this...

[ry]

I've added the BUILD.gn file... it's a bit hacky but ./tools/build.py && ./tools/test.py -p 034 works.

[ry]

This is unfortunate...

[afinch7]

It is, but it works. Any idea what is up with the appveyor build?

[afinch7]

tools/build.py --release works fine on windows, but tools/build.py doesn't for some reason.

[afinch7]

This patch resolves the issue with windows, but it wouldn't really be a debug build anymore

diff --git a/config/BUILD.gn b/config/BUILD.gn
index 3f37c75c1..d41dd38ef 100644
--- a/config/BUILD.gn
+++ b/config/BUILD.gn
@@ -148,7 +148,6 @@ config("feature_flags") {
 
 config("debug") {
   defines = [
-    "_DEBUG",
     "DYNAMIC_ANNOTATIONS_ENABLED=1",
     "WTF_USE_DYNAMIC_ANNOTATIONS=1",
   ]
diff --git a/config/win/BUILD.gn b/config/win/BUILD.gn
index b5a58459a..9963db297 100644
--- a/config/win/BUILD.gn
+++ b/config/win/BUILD.gn
@@ -422,7 +422,7 @@ config("release_crt") {
 config("dynamic_crt") {
   if (is_debug) {
     # This pulls in the DLL debug CRT and defines _DEBUG
-    cflags = [ "/MDd" ]
+    cflags = [ "/MD" ]
   } else {
     cflags = [ "/MD" ]
   }
@@ -431,7 +431,7 @@ config("dynamic_crt") {
 config("static_crt") {
   if (is_debug) {
     # This pulls in the static debug CRT and defines _DEBUG
-    cflags = [ "/MTd" ]
+    cflags = [ "/MT" ]
   } else {
     cflags = [ "/MT" ]
   }

It seems like we need to add a arg to the linker via -Clink-args ideally near this

deno/build_extra/rust/rust.gni

Lines 247 to 252 in 1b48d67

	args += [
	"-Clinker-flavor=lld-link",
	"-Clinker=" + rebase_path(
	"//third_party/llvm-build/Release+Asserts/bin/lld-link.exe",
	root_build_dir),
	]

I.E.

if (is_debug) {
  args += "-Clink-args='/someflag'"
}

[ry]

@afinch7 You simply skip the plugin build & tests when is_win && is_debug ? I forgot that we don't even test that in the 'cargo build' we always force release

deno/tools/gn.rs

Lines 20 to 23 in 32cde32

	// On Windows, we need to link with a release build of libdeno, because
	// rust always uses the release CRT.
	// TODO(piscisaureus): make linking with debug libdeno possible.
	String::from("release")

[afinch7]

In further testing I discovered that asyncs don't work correctly. futures::task::current() is completely broken in any loaded plugins. This is a limitation of the current version of the futures crate. The only real solution here is to convert everything to use the new futures api that landed with rust 1.36.0.

[afinch7]

@ry I don't think so. Also I forgot to mention that patch is for //build. Maybe just add "cargo build -p test_plugin ${release ? '--release' : '' }" or something similar to //tools/build.py for now? Your plan is to move all builds to cargo in the future right?

[ry]

Maybe just add "cargo build -p test_plugin ${release ? '--release' : '' }" or something similar to //tools/build.py for now?

That works

[ry]

Update: I very much want to land this patch, but I think we need a bit more reorganization in core ops first. In particular I'd like to add isolate.add_op("foobar", foobar_op_creator) to allow ops to be added dynamically. Also @afinch7 has informed me that async ops aren't working, and he wants to wait until we support std::future::Future here.

[ry]

Native plugins are still something we're actively working towards, but this PR is very out of date - so I'm closing it. We'll refer back to it for dlopen bindings.