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

feat: support wrapped plugin ops #5721

Closed
wants to merge 5 commits into from

Conversation

afinch7
Copy link
Contributor

@afinch7 afinch7 commented May 21, 2020

Add support for op wrapping pattern used in cli:

#[no_mangle]
pub fn deno_plugin_init(interface: &mut dyn Interface) {
  // this won't work when `register_op` takes a function pointer(`fn`)
  interface.register_op("example", wrap_op(op_example));
}

fn wrap_op<D>(d: D) -> Box<dyn Fn(&mut dyn Interface, Value, Option<ZeroCopyBuf>) -> Op> 
where
  D: Fn(&mut dyn Interface, Value, Option<ZeroCopyBuf>) -> Op,
{
  // code for wrapping op
}

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

fixes #5478

@bartlomieju bartlomieju added cli related to cli/ dir deno_core Changes in "deno_core" crate are needed labels May 22, 2020
@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch from ebe7d23 to 8315f2b Compare June 2, 2020 17:06
@afinch7
Copy link
Contributor Author

afinch7 commented Jun 3, 2020

@piscisaureus Any idea why this only fails on windows?

@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch 2 times, most recently from d5b8f02 to e0604ad Compare June 9, 2020 14:37
@CLAassistant
Copy link

CLAassistant commented Jul 27, 2020

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

Thanks @afinch7 could you please rebase this branch and add some test case?

@afinch7
Copy link
Contributor Author

afinch7 commented Aug 27, 2020

Going to rebase this and add a test case with json ops.

@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch from e0604ad to 753dcb1 Compare August 27, 2020 21:33
@afinch7
Copy link
Contributor Author

afinch7 commented Aug 27, 2020

I decided the json op test was too complicated, so I just did something simpler.

@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch from e694c07 to 45fe95e Compare August 27, 2020 23:52
@afinch7
Copy link
Contributor Author

afinch7 commented Aug 31, 2020

I think I finally figured out what causes that segfault on windows. The default drop implementation for the closure created in cli/ops/plugin.rs for each op drops the captured plugin_lib value first. I don't know if there is a way to change the order of these actions here.

I think the "correct" solution here is to make sure that all of a plugins ops are unregistered first when it is dropped.

@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch 2 times, most recently from 5d2075e to 34c20d1 Compare September 4, 2020 14:56
@afinch7
Copy link
Contributor Author

afinch7 commented Sep 4, 2020

@bartlomieju This is ready for review.

@bartlomieju
Copy link
Member

@afinch7 thanks, I'll leave review to @piscisaureus as he's refactoring the ops now.

@piscisaureus
Copy link
Member

The issue I have with this is that I see that you're wrapping ops but I have no idea why this is needed at all..

@afinch7 afinch7 force-pushed the support_wrapped_plugin_ops branch from 34c20d1 to 8cc590f Compare September 6, 2020 16:23
@afinch7
Copy link
Contributor Author

afinch7 commented Sep 6, 2020

It's not needed, but it's not currently possible to implement anything like register_op_minimal for plugin ops. I would just use a generic in Interface, but as far as I know that's not possible.

@afinch7
Copy link
Contributor Author

afinch7 commented Sep 9, 2020

With the recent changes to the op registry I found it was actually fairly easy to implement my "correct" solution of actually unregistering a plugins ops when it is closed.

@piscisaureus piscisaureus requested review from ry and removed request for piscisaureus September 9, 2020 20:07
@kitsonk
Copy link
Contributor

kitsonk commented Oct 22, 2020

This is quite stale now. We should either rebase or close.

@bartlomieju
Copy link
Member

I'm gonna close this PR without a merge because it's stale. In light of #8490 it's not clear if we want to make those changes, remove plugins completely or rework them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir deno_core Changes in "deno_core" crate are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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