Skip to content

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor - #48703

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ljharb:symbol-descriptors
Jul 12, 2023
Merged

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor#48703
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ljharb:symbol-descriptors

Conversation

@ljharb

@ljharbljharb commented Jul 8, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Followup to #48518; fixes#48699

Happy to add a test if needed - but I both wasn't sure where it would go, and I also wasn't sure if this was worth testing since node rarely polyfills JS builtins and v8 will supply it soon enough anyways (backed by test262)

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 8, 2023
@ljharbljharb mentioned this pull request Jul 8, 2023
Closed
@ljharb
ljharbforce-pushed the symbol-descriptors branch from c379eaa to c08632cCompareJuly 8, 2023 19:03
@ljharb
ljharbforce-pushed the symbol-descriptors branch 2 times, most recently from d8e640c to c70ae63CompareJuly 8, 2023 19:09
@ljharb

ljharb commented Jul 9, 2023

Copy link
Copy Markdown
MemberAuthor

I'm assuming this Mac OS test is flaky since it timed out - could someone rerun that one if needed?

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52659/

benjamingr
benjamingr reviewed Jul 9, 2023
View reviewed changes
Comment threadlib/internal/process/pre_execution.js
// TODO(MoLow): Remove this polyfill once Symbol.dispose and Symbol.asyncDispose are available in V8.
// eslint-disable-next-line node-core/prefer-primordials
Symbol.dispose ??= SymbolDispose;
if (typeof Symbol.dispose !== 'symbol') {

@benjamingrbenjamingrJul 9, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're guaranteed this runs before any userland code right?

(Just making sure this is safe since it's weird we set up Symbol.dispose here (if someone tampered with symbol they can set up a getter that throws here) but we use ObjectDefineProperty which is a primordial that guards against this sort of tampering))

@ljharbljharbJul 9, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I’m assuming that the file called “pre execution” runs before any userland code - if not, this polyfill is highly problematic conceptually :-)

@ljharb

ljharb commented Jul 11, 2023

Copy link
Copy Markdown
MemberAuthor

Are these 3 failing tests something I need to address?

@nodejs-github-bot

nodejs-github-bot commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52696/

@MoLow

MoLow commented Jul 11, 2023

Copy link
Copy Markdown
Member

Are these 3 failing tests something I need to address?

seems like a fluke, I reran the tests

@ljharb

ljharb commented Jul 12, 2023

Copy link
Copy Markdown
MemberAuthor

@MoLow still failing; are they flaky or do i need to look into it?

@nodejs-github-bot

nodejs-github-bot commented Jul 12, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52714/

@MoLow

MoLow commented Jul 12, 2023

Copy link
Copy Markdown
Member

@MoLow still failing; are they flaky or do i need to look into it?

passed now

@MoLowMoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 12, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
@nodejs-github-bot
nodejs-github-bot merged commit c2c7260 into nodejs:mainJul 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 12, 2023

Copy link
Copy Markdown
Collaborator

Landed in c2c7260

@ljharb
ljharb deleted the symbol-descriptors branch July 12, 2023 12:02
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@ljharb@juanarbol
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
eb0aba5
Followup to #48518; fixes#48699
PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@juanarboljuanarbol mentioned this pull request Jul 13, 2023
Merged
@github-actionsgithub-actionsBot mentioned this pull request Jul 16, 2023
Open
23 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb@Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
8396660
Followup to nodejs#48518; fixesnodejs#48699
PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb@Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
9c52644
Followup to nodejs#48518; fixesnodejs#48699
PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ruyadorno

ruyadorno commented Sep 11, 2023

Copy link
Copy Markdown
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@ljharb

ljharb commented Sep 11, 2023

Copy link
Copy Markdown
MemberAuthor

@ruyadorno node 18 doesn't seem to have Symbol.dispose polyfilled, so i think this PR should have "do not backport" to <= 18?

@SimenB

SimenB commented Sep 19, 2023

Copy link
Copy Markdown
Member

node 18.18 was released today with the symbols, so maybe?

@ljharb

ljharb commented Sep 19, 2023

Copy link
Copy Markdown
MemberAuthor

oof, ok then perhaps it will indeed backport cleanly into 18.18 :-)

targos pushed a commit that referenced this pull request Nov 27, 2023
@ljharb@targos
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
e08fd98
Followup to #48518; fixes#48699
PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targostargos mentioned this pull request Nov 28, 2023
Merged
@mhofmanmhofman mentioned this pull request Dec 4, 2023
Closed
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: use correct descriptor for
8320281
Followup to #48518; fixes #48699
PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: use correct descriptor for
80fb5b8
Followup to #48518; fixes #48699
PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@lpincalpincalpinca approved these changes
@MoLowMoLowMoLow approved these changes
+1 more reviewer
@LinkgoronLinkgoronLinkgoron approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.processIssues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Symbol.{ dispose, asyncDispose } have incorrect descriptors

8 participants

@ljharb@nodejs-github-bot@MoLow@ruyadorno@SimenB@Linkgoron@benjamingr@lpinca