Add ValueSerializer, ValueDeserializer bindings for structured clone in deno

[inteon]

Hey!

I started working on this issue, meanwhile I'm trying to familiarize myself with the project.
I'm not an experienced Rust developer; so please review my code!
The code I uploaded compiles, but maybe there are still some non-best-practices in the code.

Could you give some feedback before I continue working on this issue?
The current progress:

• ValueSerializer bindings
• ValueSerializer::Delegate bindings
• ValueDeserializer bindings
• ValueDeserializer::Delegate bindings
• Added Tests
• Added default Not-Implemented exceptions for write_host_object, get_shared_array_buffer_id, ...
• Added throw_data_clone_error custom callback

[CLAassistant]

All committers have signed the CLA.

[piscisaureus]

First of all - structure clone support is high on our wish list, so your effort is very much appreciated.

Your rust code so far looks great. I'll comment in-line if I have any suggestions.

One thing though - it looks like we will also need bindings for ValueSerializer::Delegate and ValueDeserializer::Delegate. I expect that adding bindings for these Delegate classes will turn
out to be a lot more difficult than what you've done so far - I hope you're up for that challenge too!

[inteon]

Thanks for the feedback, I fixed the issues and continued programming the missing parts.
For the Value(Des|S)erializer::Delegate code I based the solution on the task.rs solution.
If you have any new feedback; feel free to share. I think adding some tests will also help fixing any remaining bugs/ problems.

[piscisaureus]

For the Value(Des|S)erializer::Delegate code I based the solution on the task.rs solution.

That is indeed what I would have suggested; I'm impressed you were able to figure it out since that solution is not so straightforward or elegant.

[inteon]

Thanks for the support; I think we are making some great progress! I was wondering with regards to cleaning up the rusty_v8 interface:

• should I always use scope: &mut HandleScope<'_> instead of *mut Isolate as function argument?
• Is it ok to return *const Object, or should this be reconverted to Local<Object>?

[piscisaureus]

• should I always use scope: &mut HandleScope<'_> instead of *mut Isolate as function argument?

*mut Isolate cannot be used safely, so indeed you'd need to use another type. Which type exactly depends on the situation. In this situation, since you need to return Local handles, &mut HandleScope<'_> is indeed the type of choice.

• Is it ok to return *const Object, or should this be reconverted to Local<Object>?

It should be converted to either Local<Object> if it's guaranteed to be non-null, or Option<Local<Object>> if it might be null. You can use HandleScope::to_local() for this; there are many examples in the code base, e.g. here.

[inteon]

• should I always use scope: &mut HandleScope<'_> instead of *mut Isolate as function argument?

*mut Isolate cannot be used safely, so indeed you'd need to use another type. Which type exactly depends on the situation. In this situation, since you need to return Local handles, &mut HandleScope<'_> is indeed the type of choice.

• Is it ok to return *const Object, or should this be reconverted to Local<Object>?

It should be converted to either Local<Object> if it's guaranteed to be non-null, or Option<Local<Object>> if it might be null. You can use HandleScope::to_local() for this; there are many examples in the code base, e.g. here.

🤔 It seems like the read_host_object function in the ValueDeserializerDelegate has the Isolate pointer as one of its arguments. How can we now convert this pointer to a HandleScope? Additionally we need this HandleScope so the return value can be converted from *const Object to Local<Object> (like you just described in your comment).

I would guess the following code would not work as this creates a new HandleScope?

let scope = &mut v8::HandleScope::new(isolate);

Instead, is there a way to retrieve the instantiated HandleScope objects or should an alternative solution be used?

[piscisaureus]

Instead, is there a way to retrieve the instantiated HandleScope objects or should an alternative solution be used?

It is possible, although there are some safety constraints to it.
The way to do it is let scope = unsafe { &mut crate::scope::CallbackScope::new(&mut *isolate);
You can see it in action here.
Note that CallbackScope derefs to HandleScope .

[inteon]

Ok thx, I cleaned up the interface. Currently, the free_buffer_memory is the only function that still uses a pointer.

[inteon]

I added an extra test that serializes a javascript object and deserializes it in another "Isolate".
It seems like SharedArrayBuffer does not work without creating a SerializerDelegate with working GetSharedArrayBufferId function. I'll try to add this to the tests too.
Feel free to add extra tests that might be needed for completeness.

It looks like we're getting close to finishing this WIP 🎉!

[piscisaureus]

It seems like SharedArrayBuffer does not work without creating a SerializerDelegate with working GetSharedArrayBufferId function. I'll try to add this to the tests too.

I think that is the most important TODO item at this point.

As you might have noticed I also have some concerns w.r.t the current "strategy" for allocating and resizing serializer buffers - I have some ideas but I need to sit down and work on the code myself for a bit (otherwise I'll end up proposing "solutions" that turn out not to work after all).

Other than that this looks pretty good and almost done.

[piscisaureus]

Other TODOs:

• throw_data_clone_error() needs HandleScope.
• write_host_object needs a reference to Serializer.
• default implementations for trait methods (??)

[inteon]

UPDATE:
TODOs:

• throw_data_clone_error() needs HandleScope.
• write_host_object needs a reference to Serializer.
• default implementations for trait methods

[piscisaureus]

Apologies for the delay, I’ll land this soon.

[bnoordhuis]

@inteon I rebased your branch to fix the merge conflicts and I took the liberty of addressing some style issues. It passes for me locally and I'll merge it when the CI is green. Nice work, thanks!

[inteon]

@bnoordhuis ok thx