denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
94.69k stars 5.26k forks source link

Expose `Deno.core.(de)serialize` as a public API #12379

Closed DjDeveloperr closed 2 years ago

DjDeveloperr commented 3 years ago

Deno.core.serialize and Deno.core.deserialize seem to serialize and deserialize v8's binary format. I'm working on something that need those functions so I have to use them from Deno.core right now, which is not a public API and considered unstable (and it also doesn't have any TS types either).

Node.js exposes this functionality through v8 module: https://nodejs.org/api/v8.html#v8_serialization_api

Could these two functions be exposed as a public API?

MierenManz commented 3 years ago

might also be a good idea to expose it as a public api now that ffi supports buffers.

kitsonk commented 2 years ago

It is very intentional that they are on Deno.core and non-public, as we really don't want people to couple to them. They are quite "advanced" APIs. The web platforms structuredClone() is the effective public API.

What is the use case?

DjDeveloperr commented 2 years ago

I need these APIs to pass and return JS values to/from a native library, structuredClone is not much helpful in this case.

andreubotella commented 2 years ago

I need these APIs to pass and return JS values to/from a native library, structuredClone is not much helpful in this case.

We don't currently support for-storage serialization, meaning that if you serialize ArrayBuffer, SharedArrayBuffer or WebAssembly.Module objects and then try to deserialize that from a different process, you'll get a panic.

DjDeveloperr commented 2 years ago

From what I tested it seems serializing ArrayBuffer works, except SharedArrayBuffer and such which use a transfer ID would of course not work. Aside, native library is on same process I think, it's loaded through the new FFI API.

Note: I chose this binary format over JSON because it's easier to parse and likely more efficient, and it supports (de)serializing more type of values, of which I need at the moment is Array Buffer and Views.

andreubotella commented 2 years ago

I misspoke, serializing ArrayBuffer objects will work, but transferring them won't, because they do use a transfer ID to allow the zero-copy transfer.

Regardless, making Deno.core.(de)serialize available as a public API would expose users to the internals of how transfer IDs work, and might allow them to trigger UB if a transferred ArrayBuffer is deserialized multiple times, which is something the message passing and structured clone APIs don't allow.

DjDeveloperr commented 2 years ago

Seems like Node.js straight away errors that SharedArrayBuffer cannot be cloned, I think we can do same to avoid UB?

andreubotella commented 2 years ago

Seems like Node.js straight away errors that SharedArrayBuffer cannot be cloned, I think we can do same to avoid UB?

That would no longer be exposing the serialization internals directly, but a version of for-storage (de)serialization, which is used in the web platform for storing serialized values in persistent storage. We don't yet implement for-storage serialization, since Deno CLI doesn't yet implement Indexed DB or any of the other web APIs that use it – but we will probably need to implement it for denoland/deno#10750.

So this would be technically feasible. I'll leave it to the core team to decide whether it's worth exposing or not.

petamoriken commented 2 years ago

related https://github.com/whatwg/html/issues/3517

kitsonk commented 2 years ago

That is pretty stale, and with structured clone having moved forward, I suspect it is unlikely that it will ever get standardized, at it would require agreement on what the serialization format would be, which would be very specific to the engine, or agreement of the least worst version.

Of course if something gets agreement we would be glad to support it, but I wouldn't hold your breath.

cjihrig commented 2 years ago

Another reason not to make this public API: V8 sometimes changes the serialize/deserialize format in ways that would be difficult for us to maintain. This has impacted Node.js and Cloudflare Workers already.

MierenManz commented 2 years ago

I still feel like eventho it has breaking changes because of a format that is not backwards compatible that it can still be useful in the context of passing stuff from and to shared libraries via the FFI

But in that regard I have a repo that supports the latest format used for Deno.core.deserialize and Deno.core.serialize so technically there is not a need to support it from deno's perspective.

kitsonk commented 2 years ago

There are many arguments for not exposing it publicly. Respectfully declining.

johnspurlock commented 1 year ago

With the advent of Deno KV and KV Connect, could you reconsider this?

Working with v8-persisted KV values in userland in other Deno processes would benefit greatly from this!