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

src: do not rely on the internal field being default to undefined #49413

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

joyeecheung
Copy link
Member

It's an undocumented V8 behavior that is subject to change. Instead just check if the internal field is set to a promise. There is also no need to check IsEmpty() since the object is guaranteed to be constructed by the FileHandle constructor with enough internal fields.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/

It's an undocumented V8 behavior that is subject to change. Instead
just check if the internal field is set to a promise. There is also
no need to check IsEmpty() since the object is guaranteed to be
constructed by the FileHandle constructor with enough internal
fields.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 30, 2023
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49413
✔  Done loading data for nodejs/node/pull/49413
----------------------------------- PR info ------------------------------------
Title      src: do not rely on the internal field being default to undefined (#49413)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:fd-internal-field -> nodejs:main
Labels     c++, fs, needs-ci
Commits    1
 - src: do not rely on the internal field being default to undefined
Committers 1
 - Joyee Cheung 
PR-URL: https://github.com/nodejs/node/pull/49413
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49413
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 30 Aug 2023 21:58:27 GMT
   ✔  Approvals: 1
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49413#pullrequestreview-1603680709
   ✘  This PR needs to wait 6 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-09-02T20:15:12Z: https://ci.nodejs.org/job/node-test-pull-request/53698/
- Querying data for job/node-test-pull-request/53698/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6099430352

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 7, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2023
@nodejs-github-bot nodejs-github-bot merged commit 941ad8b into nodejs:main Sep 7, 2023
43 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 941ad8b

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
It's an undocumented V8 behavior that is subject to change. Instead
just check if the internal field is set to a promise. There is also
no need to check IsEmpty() since the object is guaranteed to be
constructed by the FileHandle constructor with enough internal
fields.

PR-URL: #49413
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
It's an undocumented V8 behavior that is subject to change. Instead
just check if the internal field is set to a promise. There is also
no need to check IsEmpty() since the object is guaranteed to be
constructed by the FileHandle constructor with enough internal
fields.

PR-URL: nodejs#49413
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants