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

lib: decorate undici classes as platform interfaces #55178

Closed
wants to merge 1 commit into from

Conversation

jazelly
Copy link
Member

@jazelly jazelly commented Sep 30, 2024

Node recognizes platform/host objects by counting internal slots.
Undici, as a downstream module, does not have access to internal slots,
and hence its instances are recognized as plain objects. This caused
issues on the structureClone algorithm, which has few restrictions on
non-platform objects.

This PR tries to fix it by decorating Undici classes with the internal
slots so that underlying Serialize() can recognize its instances as
host objects.

On another note, this PR consolidates the lazy loading of Undici, so
that the proxied Undici classes are referential equal.

Fixes: #55120

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 30, 2024
@jazelly jazelly force-pushed the fix-55120 branch 2 times, most recently from 663ec95 to e91bfab Compare September 30, 2024 13:48
lib/internal/util.js Outdated Show resolved Hide resolved
@jazelly jazelly changed the title lib: decorate undici as platform interface lib: decorate undici classes as platform interfaces Sep 30, 2024
@anonrig anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 30, 2024
@jakecastelli jakecastelli added the web-standards Issues and PRs related to Web APIs label Sep 30, 2024
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (89a2f56) to head (2fffbee).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55178      +/-   ##
==========================================
+ Coverage   88.39%   88.41%   +0.01%     
==========================================
  Files         652      652              
  Lines      186565   186591      +26     
  Branches    36046    36051       +5     
==========================================
+ Hits       164916   164971      +55     
+ Misses      14908    14896      -12     
+ Partials     6741     6724      -17     
Files with missing lines Coverage Δ
lib/http.js 100.00% <100.00%> (ø)
lib/internal/util.js 97.67% <100.00%> (+0.82%) ⬆️
lib/internal/wasm_web_api.js 100.00% <100.00%> (ø)

... and 33 files with indirect coverage changes

Node recognizes platform/host objects by counting internal slots.
Undici, as a downstream module, does not have access to internal slots,
and hence its instances are recognized as plain objects. This caused
issues on the `structureClone` algorithm, which has few restrictions on
non-platform objects.

This PR tries to fix it by decorating Undici classes with the internal
slots so that underlying `Serialize()` can recognize its instances as
host objects.

On another note, this PR consolidates the lazy loading of Undici, so
that the proxied Undici classes are referential equal.
@mcollina
Copy link
Member

mcollina commented Oct 1, 2024

I'm a bit at a loss in why we want to keep that symbol private. Wouldn't it benefit everyone to allow a public way to configure this?

On the issue, I think we might be better off in applying this change during the "build"
of undici, so we avoind the performance penalty of Proxy+ReflectConstruct.

@jazelly
Copy link
Member Author

jazelly commented Oct 1, 2024

why we want to keep that symbol private

FWIW, we can expose the symbol, but userland can mutate that value and make an object that was supposed to be transferable but now non-transferable, or vice versa. Although this is not unprecedented (think about the concept [[Prototype]] and the accessor __proto__), we made a lot of efforts to make node internal safe because of that.

I am open to suggestions for this one, but I kept it as is in this PR as it looks like from the initial design, and changing that I think is pending a decision.

we might be better off in applying this change during the "build" of undici

This is achievable once the symbol is public accessible.

so we avoid the performance penalty of Proxy+ReflectConstruct

My understanding is the performance penalty is there but not much from this fix, as the module is lazy loaded once only. The benchmark CI should provide more insights.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

I believe undici can use this public API to mark an object as not transferable: https://nodejs.org/api/worker_threads.html#workermarkasuntransferableobject.

A new API can be added to mark an object as not cloneable, like worker.markAsUncloneable(object), along side with https://nodejs.org/api/worker_threads.html#workermarkasuntransferableobject..

@jazelly
Copy link
Member Author

jazelly commented Oct 1, 2024

Ah, that will work better. I didn't know that. I think this PR should be closed then.

I can raise a new PR to add that API.

@jazelly jazelly closed this Oct 1, 2024
nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2024
External modules need a way to decorate their objects so that node can
recognize it as a host object for serialization process. Exposing a way
for turning off instead of turning on is much safer.

PR-URL: #55234
Refs: #55178
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
targos pushed a commit that referenced this pull request Oct 5, 2024
External modules need a way to decorate their objects so that node can
recognize it as a host object for serialization process. Exposing a way
for turning off instead of turning on is much safer.

PR-URL: #55234
Refs: #55178
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
External modules need a way to decorate their objects so that node can
recognize it as a host object for serialization process. Exposing a way
for turning off instead of turning on is much safer.

PR-URL: nodejs#55234
Refs: nodejs#55178
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
External modules need a way to decorate their objects so that node can
recognize it as a host object for serialization process. Exposing a way
for turning off instead of turning on is much safer.

PR-URL: nodejs#55234
Refs: nodejs#55178
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@jazelly jazelly deleted the fix-55120 branch November 25, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

structuredClone Serializing a non-serializable platform object succeeds
7 participants