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(ext/ffi): Callbacks #14663

Merged
merged 72 commits into from
Jun 20, 2022

Conversation

aapoalas
Copy link
Collaborator

@aapoalas aapoalas commented May 18, 2022

This branch implements APIs through which users can register FFI callbacks, ie. C callbacks, for use in existing Deno FFI APIs.

Closes #13186

The API usage is as follows:

const lib = Deno.dlopen("path", {
  setQueueCallback: {
    parameters: [{ function: { parameters: ["i32"], result: "void" }}],
    result: "void",
  },
});
type i32 = number;
function queueCallback(status: i32) {
  console.log(status);
};
const queueCallbackResource = new Deno.RegisteredCallback({
  parameters: ["i32"],
  result: "void",
}, queueCallback);
lib.symbols.setQueueCallback(queueCallbackResource);
// After done
queueCallbackResource.close();

Possible improvements

  • The format for declaring function type parameters has two objects. I'd like to turn this into a tagged object eg. { "type": "function", "parameters": [], "result": "void" } but didn't find a way to serde work with me on that.
  • Currently, setting the parameter and result types on a registered callback doesn't really do anything. These could be used to check a function parameter for matching signatures.
  • Currently the PR allows for pointers received from FFI calls to be passed as function type parameters. This may be useful in some cases but is also likely to be a very fun footgun.
    • I think it's possible to load FFI functions as pointer type symbols. These could then be passed back as function type parameters. I think FFI is at least close to being strong enough in complexity to implement itself.
  • Eventually, I'd want to have the registered callbacks be garbage collectable resources.

Steps to completion

  • Fix nonblocking FFI calls
    • Hybrid ops
  • Tests
  • RegisteredCallback TS type
  • Type tests

Ready to merge

@aapoalas aapoalas force-pushed the feat/ffi-register-callback-rewrite branch from dfba80a to 69633f0 Compare May 21, 2022 16:43
@aapoalas aapoalas changed the title feat(ffi): Explicit callbacks feat(ext/ffi): Explicit callbacks May 22, 2022
@aapoalas aapoalas force-pushed the feat/ffi-register-callback-rewrite branch 2 times, most recently from 5530081 to b7821f3 Compare June 8, 2022 04:30
@aapoalas aapoalas force-pushed the feat/ffi-register-callback-rewrite branch from ad6e676 to 69ae9bf Compare June 8, 2022 12:32
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@littledivy
Copy link
Member

We're going to further iterate on the API and performance. This is a great first pass

@aapoalas aapoalas changed the title feat(ext/ffi): Explicit callbacks feat(ext/ffi): Callbacks Jun 16, 2022
@aapoalas
Copy link
Collaborator Author

Good to merge on my part.

@piscisaureus
Copy link
Member

  • The format for declaring function type parameters has two objects. I'd like to turn this into a tagged object eg. { "type": "function", "parameters": [], "result": "void" } but didn't find a way to serde work with me on that.

Did #[serde(tag = "type")] not work?

@@ -346,9 +341,14 @@ declare namespace Deno {
| "f64"
| "pointer";

type NativeParameterType =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking, maybe it makes things more explicit if we had NativeParameterType and NativeReturnType, as in:

  • NativeParameterType can be sent from JS to C
  • NativeReturnType can be sent from C to JS

Of course in case of callbacks the types would be reversed: parameters would be NativeReturnType and the return value would have type NativeParameterType.

So maybe we should considerToNativeType and FromNativeType or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and do plan on doing a rework on the FFI TS types. f.ex. Currently NativeType is the "base type" and NativeNumberType is a narrowed down type from that. I will do a follow-up to rework the types into something that hopefully resembles a reasonable whole.

A ToNativeType and FromNativeType sounds splendid for that.

? void
: T extends StaticNativeBigIntType ? number | bigint
: T extends StaticNativeNumberType ? number
: T extends "pointer" ? UnsafePointer | TypedArray | null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use TypedArray, who ends up owning the memory? How is it released?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am by no means an expert, but from my understanding (or lack of it) of the C ABI (or lack of it :D ) there is no way for a function call to pass actual ownership of memory.

  1. Pointers are of course purely pointers to the memory. The memory itself is still owned by whoever allocated it, and the caller and callee must agree on what is found at the other end of the pointer.
  2. Structs (which the FFI API currently does not actually support) pass struct parts through various registers (and others on some platforms), but these are still copies of the actual data in memory, the memory itself is still allocated and released by the same party.

Of course with structs (and even with pointers) there's a possibility that whatever is contained in the struct has pointers that should be considered to carry ownership with it. An example would be the result of Rust Box's Box::to_raw_parts() which returns essentially a (*const u8, usize, usize) struct. This struct would likely get passed in through FFI in a bunch of registers. From Deno FFI point of view this would be a new BigIntArray(3). Now the question is, what happens to the three BigInts when this struct gets passed through an FFI boundary?

  1. The values of the usize integers get copied to the other side of the FFI boundary.
  2. The ownership of the (implied) Box is transferred to the other side of the FFI boundary.
  3. The receiver of ownership is expected to call any required APIs when it is done with the ownership. Of course just removing the usize integers isn't going to be enough. Releasing whatever is found on the other side of pointers needs to be done according to the original allocators preferences. Generally this means calling a destructor / drop implementation.
  4. The values on the original side of the FFI boundary, what happens to them? They are not changed, of course. Three (essentially) usize integers are not going to changed or deallocated. What they represent, that is the important part but it's also beyond what FFI can represent: The understanding that the first *const u8 is a pointer referring to a contiguous memory of some length determined by the value of one of the usize parameters, and that whoever has last received that pointer should carry the responsibility of calling a destructor when they're done with it is plainly a contract, not one built into the ABI itself.

So, when a TypedArray is used as a pointer, the ownership of the memory (as a physical address space) remains always with Deno, and by extension the user of the FFI library. The user must make sure that the TypedArray does not go out of scope and get garbage collected for as long as the pointer is given to an FFI library. Deno's FFI can only provide the means by which the contract of memory usage is shared (passing pointers to memory), it cannot (with the current API at least) accurately represent passing of ownership.

If / once structs are supported (this is another planned followup of mine), those will likely operate on a different logic. Whereas pointers are just referring to memory, structs are generally understood to pass ownership. On a technical level the data is still copied, but eg. if a struct contains a pointer (like with the above Box example) then it is generally understood that the ownership is passed along with the struct. For this reason, I think the proper model for struct passing is definitely detaching ArrayBuffers.

So, where passing a TypedArray as pointer means that Deno must keep the buffer alive then on the other hand passing a TypedArray as struct would generally mean that any ownership implied by the buffer are transferred to the other side of the FFI boundary. As such, it's generally more apt that the buffer ownership is taken away from Deno and passed onto Rust (which will then deallocate it according to the ArrayBuffer's deleter callback). If then user code truly wants to retain part-ownership of the struct contained in the TypedArray they'll need to either plainly copy the buffer or (more likely) call some native library's copy constructor with another TypedArray argument into which the copy will be created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr ownership always resides with the backing store managed by V8.

# Conflicts:
#	ext/ffi/00_ffi.js
#	test_ffi/tests/test.js
@aapoalas
Copy link
Collaborator Author

Did #[serde(tag = "type")] not work?

I'll have to recheck, but when I tried originall serde did not like mixing untagged (plain strings) and internally tagged (tag = "type") enum members, or so it seemed.

@aapoalas
Copy link
Collaborator Author

Did #[serde(tag = "type")] not work?

Okay, I tested this and indeed there doesn't seem to be a clear way to do this.

We'd need the same enum to handle both of these: "u8" and { "function": { "parameters": [...], "result": "..." } } or { "type": "function", "parameters": [...], "result": "..." }. If serde(tag = "type") is used then it would imply that U8 should be presented in JSON as { type: "u8" }. On the other hand, If we want { type: "function", parameters: [...], result: "..." } to get parsed then that would imply the serde(tag = "type") setting.

So, these two obviously do not coexist. Or at least I am not aware of a way for the two to coexist. Luckily there's also a benefit for the current predicament. Function {} requires the JSON format to be { "function": { ... } }, where we of course use the { ... } to be { "parameters": [...], "result": "..." }. That object format then happens to exactly align with the declaration that we expect UnsafeCallback to be given, meaning that if so wanted FFI users can reuse the same objects for both callback parameter declaration AND the UnsafeCallback declaration that will eventually get passed as the parameter.

@littledivy
Copy link
Member

Yeah, cannot mix different tag representations in serde.

@nayeemrmn
Copy link
Collaborator

Even for the API, mixing "u8" and { type: "function", ... } doesn't seem desirable at all, since it's an inconsistent tagging scheme. That would be the third best option after (1) what we have now aligning with serde's defaults and (2) internally tagged as { type: "u8" } + { type: "function", ... }.

@aapoalas
Copy link
Collaborator Author

Even for the API, mixing "u8" and { type: "function", ... } doesn't seem desirable at all, since it's an inconsistent tagging scheme. That would be the third best option after (1) what we have now aligning with serde's defaults and (2) internally tagged as { type: "u8" } + { type: "function", ... }.

Yeah, I originally wanted to avoid the extra object needed for { function: {} }, but now I'm very happy with it, since as said it allows the inner object to be reused as-is for both the parameter and the UnsafeCallback's definition.

@littledivy littledivy merged commit 3d6fa64 into denoland:main Jun 20, 2022
@aapoalas aapoalas deleted the feat/ffi-register-callback-rewrite branch June 20, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does deno need a similar API to support and FFI callbacks
4 participants