[Plugins] non static functions cannot be used for plugin ops

[afinch7]

There doesn't appear to be a way to make our existing pattern of op wrappers work with plugin ops.

// example function that generates a function based on input.
pub fn wrap_op<D>(d: D) -> impl Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op
where
  D: Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op,
{
  // wrapper code
}

pub fn op_example(
  _interface: &mut dyn Interface,
  data: &[u8],
  zero_copy: Option<ZeroCopyBuf>,
) -> Op {
  // does something
}

#[no_mangle]
pub fn deno_plugin_init(interface: &mut dyn Interface) {
  // This will not work because `warp_op` returns a `Fn`(trait) value where
  // `register_op` requires a `fn`(static function pointer)
  interface.register_op("example", wrap_op(op_example));
  // This works fine because it's a static function
  interface.register_op("example", op_example);
  // It will also work fine with static closures.
}

We can't just change the signature of Interface::register_op and use a Fn as it's a trait and isn't sized. We also can't use a generic here if we want to use Interface as a object(&mut dyn Interface). We can use Box<Fn>, but I would rather not as that diverges from normal ops.

[piscisaureus]

If we really wanted to then we could change the interface to take a trait object:

fn register_op(
  &mut self,
  name: &str, 
  dispatcher: &dyn Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op
) -> OpId;

But I'm not really sure that's necessary. The important difference between a &dyn Fn and function pointer is that the former is a closure and therefore it can capture variables, whereas a function pointer cannot. So you should ask yourself -- does wrap_op() need to capture variables in a closure. If not, I'm sure wrap_op can be changed to return a regular function pointer; we do it all the time in rusty_v8, perhaps most elegantly exemplified here: https://github.com/denoland/rusty_v8/blob/36bec8cd2ba766b9428cc0fb3f2038863e7c21ff/src/isolate.rs#L391-L406.

TLDR: unless you need to capture variables, this is possible:

pub fn wrap_op<D>(d: D) -> fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op
where
  D: Fn(&mut C, &[u8], Option<ZeroCopyBuf>) -> Op,
{
  // wrapper code
}

[piscisaureus]

One thing that the current interface does not support is for the plugin to have some "state" that persists between op calls. Using a function trait for op dispatchers might be the solution, although we'd need to use &mut FnMut in that case, and it'd be difficult to deal with the lifetime constraints because any captured variable would have to outlive deno_plugin_init().

I think adding functionality to trait Interface to support setting/getting plugin specific persistent state would be a more logical way forward.

[afinch7]

So you should ask yourself -- does wrap_op() need to capture variables in a closure.

I figure it would be pretty hard to wrap a function without capturing said function.

&dyn Fn(&mut dyn Interface, &[u8], Option) -> Op

Would require a static lifetime for that reference, so not really ideal. Unless there is some simple way to provide a static reference to a closure. Edit: it's possible to avoid static, but as you said " difficult to deal with the lifetime constraints".

Are there any other reasons to avoid Box?

Also for a little context I'm working on a common version of dispatch_json for both plugins and normal ops(see #4434).

[piscisaureus]

I figure it would be pretty hard to wrap a function without capturing said function.

But it is possible (thanks to rusts "function item types").
If you show me some code you're having trouble with, I'll show you how it's done.

[afinch7]

The main real example I can find is the dispatch_json wrapper function(ignore that fact that it takes &mut CoreIsolate):

deno/cli/ops/dispatch_json.rs

Lines 47 to 101 in aa1284c

	pub fn json_op<D>(
	d: D,
	) -> impl Fn(&mut CoreIsolate, &[u8], Option<ZeroCopyBuf>) -> Op
	where
	D:
	Fn(&mut CoreIsolate, Value, Option<ZeroCopyBuf>) -> Result<JsonOp, OpError>,
	{
	move |isolate: &mut CoreIsolate,
	control: &[u8],
	zero_copy: Option<ZeroCopyBuf>| {
	let async_args: AsyncArgs = match serde_json::from_slice(control) {
	Ok(args) => args,
	Err(e) => {
	let buf = serialize_result(None, Err(OpError::from(e)));
	return Op::Sync(buf);
	}
	};
	let promise_id = async_args.promise_id;
	let is_sync = promise_id.is_none();
	let result = serde_json::from_slice(control)
	.map_err(OpError::from)
	.and_then(|args| d(isolate, args, zero_copy));
	// Convert to Op
	match result {
	Ok(JsonOp::Sync(sync_value)) => {
	assert!(promise_id.is_none());
	Op::Sync(serialize_result(promise_id, Ok(sync_value)))
	}
	Ok(JsonOp::Async(fut)) => {
	assert!(promise_id.is_some());
	let fut2 = fut.then(move |result| {
	futures::future::ready(serialize_result(promise_id, result))
	});
	Op::Async(fut2.boxed_local())
	}
	Ok(JsonOp::AsyncUnref(fut)) => {
	assert!(promise_id.is_some());
	let fut2 = fut.then(move |result| {
	futures::future::ready(serialize_result(promise_id, result))
	});
	Op::AsyncUnref(fut2.boxed_local())
	}
	Err(sync_err) => {
	let buf = serialize_result(promise_id, Err(sync_err));
	if is_sync {
	Op::Sync(buf)
	} else {
	Op::Async(futures::future::ready(buf).boxed_local())
	}
	}
	}
	}
	}

I think a simpler example might be: a function that returns a closure that captures a owned immutable value.

fn generate_op(result: &[u8]) -> impl Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op {
  move |_interface: &mut dyn Interface,
  data: &[u8],
  zero_copy: Option<ZeroCopyBuf>| -> Op {
    if let Some(buf) = zero_copy {
    let data_str = std::str::from_utf8(&data[..]).unwrap();
    let buf_str = std::str::from_utf8(&buf[..]).unwrap();
    println!(
      "Hello from plugin. data: {} | zero_copy: {}",
      data_str, buf_str
    );
  }
  let result_box: Buf = Box::new(*result); // Note usage of result
  Op::Sync(result_box)
  }
}

Is there some way to return a fn here instead? Would it make any difference if result was a function instead? Could I still call that function in my closure?

[piscisaureus]

Is there some way to return a fn here instead?

No, that is not possible.

Would it make any difference if result was a function instead? Could I still call that function in my closure?

Yes, then it would be possible.

[piscisaureus]

BTW, I've been looking at #4434.
Boxing plugin op dispatcher functions seems fine to me, but I wonder if we can can do the actual boxing in plugin.rs, rather than making the plugin implementer do it. That's because:

pub trait DispatchOpFn:
  Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op
{
  fn box_op(self) -> Box<dyn DispatchOpFn>;
}

^-- That box_op() function is pretty ugly.

[afinch7]

Turns out that using Box causes a segfault whenever a plugin is loaded and a JS error is thrown. More specifically it's a call to drop_in_place<alloc::boxed::Box<DispatchOpFn>>() that gives the segfault. Update: found that the library was being unloaded early when the resource table is dropped. I'm not sure why there should still be several copies of the reference counter.

It also causes what I assume is a different segfault on windows.

[stale]

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.