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

web: mark as uncloneable when possible #3709

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Oct 10, 2024

Rationale

This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw DataCloneError.

This is a follow-up work from nodejs/node#55234

Changes

Marked the following as uncloneable.

'FormData', 'Headers', 'Request', 'Response', 'MessageEvent', 'CloseEvent',  'EventSource', 'WebSocket'

Features

Bug Fixes

Breaking Changes and Deprecations

Status

This tells node that the marked instances from undici are not cloneable,
so that attempts to cloning those throw `DataCloneError`.
@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

I am hesitant to write a test here, as it would be like testing the behaviour of structuredClone, which should be placed at Node. I will do a follow up once this is bumped into node.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

I really think having a test would be useful, add it.

@ronag ronag requested a review from KhafraDev October 10, 2024 11:37
@ronag
Copy link
Member

ronag commented Oct 10, 2024

Why is Headers unclonable?

@ronag ronag closed this Oct 10, 2024
@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

AFAIK, any cloneable that wants to be cloned in node runtime needs to

  1. implement kClone
  2. implement kDeserialize
  3. markTransferMode() as "cloneable"

One example you can find in node is https:/nodejs/node/blob/00b2f07f9ddeb8ffd2fb2108b0ed9ffa81ea000d/lib/internal/webstreams/transfer.js#L49

I don't think the above are exposed, so undici classes are not cloneable in node from that sense.

@ronag ronag reopened this Oct 10, 2024
@ronag
Copy link
Member

ronag commented Oct 10, 2024

I didn't mean to close this. Sorry.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 10, 2024

How can we access those symbols from userland?

@KhafraDev
Copy link
Member

Why is Headers unclonable?

@ronag it's because webidl platform objects cannot be cloned:

A JavaScript value value is a platform object if Type(value) is Object and if value has a [[PrimaryInterface]] internal slot.

Platform objects can be serializable objects if their primary interface is decorated with the [Serializable] IDL extended attribute. Such interfaces must also define the following algorithms:

Notice that the webidl for Headers does not include serializable:

[[Exposed](https://webidl.spec.whatwg.org/#Exposed)=(Window,Worker)]
interface Headers {
  ...
}

lib/core/util.js Outdated Show resolved Hide resolved
@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

I should've mentioned, this is more to tell Node to treat undici instances as a platform object, as this api will attach a private attribute that Node can recognize. Without it, Node will try to find the kTransfer method. If it's not present, it will return {}, which is why we had the original issue on node side.

structuredClone(new Response()) // {}

I think whether/how we make some classes cloneable in undici should be a separate and broader discussion, since it might involve whether/how Node exposes its internal symbols.

@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

I also added the test. It works fine on my local but won't be effective on current CI, as the API hasn't been released. The API is on v22.10 release line, so I am happy to hold this PR till then if that's more applicable.

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

why "maybeMarkAsUncloneable"? If in the future all supported platforms have markAsUncloneable, then do we need to rename this function and all the places where it is called?

More likely that we forget that in 3 years.

lib/web/fetch/webidl.js Outdated Show resolved Hide resolved
@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

More likely that we forget that in 3 years.

Good point. I updated the naming.

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

RSLGTM

btw. cloneable or clonable, is the spelling right?

@jazelly
Copy link
Contributor Author

jazelly commented Oct 10, 2024

Yeah, it's correct. In node it's always cloneable.

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

Does this also need to be done for EventSource, WebSocket, WebsocketStream, CloseEvent, ErrorEvent, MessageEvent, Cache, CacheStorage, and possibly others?

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.

5 participants