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

Implement globalThis.structuredClone #39713

Closed
targos opened this issue Aug 9, 2021 · 9 comments
Closed

Implement globalThis.structuredClone #39713

targos opened this issue Aug 9, 2021 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@targos
Copy link
Member

targos commented Aug 9, 2021

See whatwg/html#3414

@targos targos added the feature request Issues that request new features to be added to Node.js. label Aug 9, 2021
@MylesBorins
Copy link
Contributor

YES PLEASE!

@targos targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 11, 2021
@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Aug 11, 2021

Would love to give this a shot! I have some time this week I can get an initial PR up 🚀

@Ethan-Arrowood
Copy link
Contributor

Doing some pre investigation for this feature, I found this existing structuredClone method in lib/internal/util.js

node/lib/internal/util.js

Lines 456 to 469 in 33f9b7f

function structuredClone(value) {
const {
DefaultSerializer,
DefaultDeserializer,
} = require('v8');
const ser = new DefaultSerializer();
ser._getDataCloneError = hideStackFrames((message) =>
lazyDOMException(message, 'DataCloneError'));
ser.writeValue(value);
const serialized = ser.releaseBuffer();
const des = new DefaultDeserializer(serialized);
return des.readValue();
}

Should I look to modify this?

It seems to only be used in lib/internal/perf/usertiming.js

structuredClone(detail) :


Additionally, two implementation questions for StructuredSerializeWithTransfer function:

  1. How do i check for internal slots?
  2. How do I throw a "DataCloneError" DOMException?

For each transferable of transferList:

If transferable has neither an [[ArrayBufferData]] internal slot nor a [[Detached]] internal slot, then throw a "DataCloneError" DOMException.

If transferable has an [[ArrayBufferData]] internal slot and ! IsSharedArrayBuffer(transferable) is true, then throw a "DataCloneError" DOMException.

If memory[transferable] exists, then throw a "DataCloneError" DOMException.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2021

/cc @addaleax ... she could likely provide a lot more detail and context on these bits than anyone else.

@addaleax
Copy link
Member

@Ethan-Arrowood Yeah, so … for a bit of context, the serialization/deserialization API in the v8 module has been around for quite a while, and predates the addition of SharedArrayBuffers, MessagePorts and Worker Threads.

Because the two APIs are used a bit differently, they have diverged a bit; in particular, MessagePort is the API where most of the transfer/clone functionality for host objects has been implemented, whereas the v8 module one is a bit more limited when it comes to that.

Specifically, the v8 module API always encodes into a binary serialization; this also means that, for host objects (i.e. those with internal slots), the v8 module API does not know how to serialize transferable Node.js API objects properly (e.g. FileHandle or MessagePort) because they have internal state that might just not be representable as binary data. It also misses support for things like SharedArrayBuffers altogether.

I think a drop-in structuredClone for Node.js could be:

{
  const { MessageChannel, receiveMessageOnPort } = require('worker_threads');
  const channel = new MessageChannel();
  channel.unref();
  globalThis.structuredClone = function structuredClone(value, transfer) {
    channel.port1.postMessage(value, transfer);
    return receiveMessageOnPort(channel.port2).message;
  }:
}

I think that would actually do well enough in terms of performance and correctness to be added as an initial implementation. It’s obviously not ideal to create two internal MessagePort objects here, but adjusting the v8 module’s API would take quite some time and effort, I assume.

@addaleax
Copy link
Member

Also, as for the questions above:

  • How do i check for internal slots?

Generally speaking, that’s done through the InternalFieldCount() method in the V8 API, but in the V8 ValueSerializer API, it’s WriteHostObject() that lets us override behavior for these objects.

  • How do I throw a "DataCloneError" DOMException?

I think the straightforward answer here is ThrowDataCloneException() from node_messaging.cc?

Also, feel free to reach out in some other way if you’d want to have a synchronous conversation about this in some way 🙂

@Ethan-Arrowood
Copy link
Contributor

Thank you @addaleax ! I'll give your solution a shot for now, and then go from there. There appears to be some WPT tests for the new spec; hopefully, those will let us know if things are working or not 😁

@CarterLi
Copy link

Found something about receiveMessageOnPort

#34355 (comment)

@fregante
Copy link

fregante commented Nov 8, 2021

Will this be back ported to v16? #40756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants