Skip to content

src: lock the isolate properly in IsolateData destructor#57031

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 2 commits into from
Feb 15, 2025
Merged

src: lock the isolate properly in IsolateData destructor #57031

nodejs-github-bot merged 2 commits into from
Feb 15, 2025

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung commented Feb 13, 2025

Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread.

@joyeecheung
src: lock the isolate properly in IsolateData destructor
a597b98
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread.
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 13, 2025
@joyeecheungjoyeecheung mentioned this pull request Feb 13, 2025
Closed
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 13, 2025

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

@targos
Copy link
Member

targos commented Feb 13, 2025

Looks like we can't do that on the main branch. Feel free to push the commit to #56959

@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Feb 13, 2025

It seems it also fails in the V8 13.3 branch in the cctest, I suspect that this is also exposing that we aren't using HandleScope properly throughout the codebase.

@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Feb 13, 2025•
edited
Loading

Looks like it's the cctests not locking threads and the exposure kinda spreads once someone is locking the threads. The second commits fix them locally for me.

@targos
Copy link
Member

targos commented Feb 13, 2025

@nodejs/cpp-reviewers

@codecov
Copy link

codecovbot commented Feb 13, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (85f5a6c) to head (7bfd05c).
Report is 21 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57031 +/- ## ======================================= Coverage 89.11% 89.11% ======================================= Files 665 665 Lines 193193 193194 +1 Branches 37212 37212 ======================================= + Hits 172158 172168 +10 + Misses 13775 13774 -1 + Partials 7260 7252 -8
Files with missing linesCoverage Δ
src/env.cc85.89% <100.00%> (-0.16%)⬇️

... and 29 files with indirect coverage changes

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 14, 2025•
edited by jasnell
Loading

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

@joyeecheungjoyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 15, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 15, 2025
@nodejs-github-botnodejs-github-bot merged commit d384151 into nodejs:mainFeb 15, 2025
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2025

Landed in d384151

targos pushed a commit that referenced this pull request Feb 17, 2025
@joyeecheung@targos
src: lock the isolate properly in IsolateData destructor
fa26f83
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: #57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@joyeecheung@acidiney
src: lock the isolate properly in IsolateData destructor
35f5de2
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: nodejs#57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Feb 25, 2025
Merged
aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@joyeecheung@aduh95
src: lock the isolate properly in IsolateData destructor
6732f3d
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: #57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@joyeecheung@aduh95
src: lock the isolate properly in IsolateData destructor
b2c3d8d
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: #57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@aduh95aduh95 mentioned this pull request Apr 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@joyeecheung@RafaelGSS
src: lock the isolate properly in IsolateData destructor
60a645f
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: #57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@joyeecheung@RafaelGSS
src: lock the isolate properly in IsolateData destructor
619e52c
Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread. PR-URL: #57031 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Apr 25, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
073a033
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request Apr 25, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
e7a8909
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 1, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
0db4eb1
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 5, 2025
@electron-roller@codebytere
chore: bump node to v22.15.0 (main) (#46742)
cb445b3
* chore: bump node in DEPS to v22.15.0 * inspector: fix GN build nodejs/node#56798 * test: search cctest files nodejs/node#56791 * crypto: fix missing OPENSSL_NO_ENGINE guard nodejs/node#57012 * test,crypto: make tests work for BoringSSL nodejs/node#57021 * module: use synchronous hooks for preparsing in import(cjs) nodejs/node#55698 * deps: update simdjson to 3.12.0 nodejs/node#56874 * build: remove explicit linker call to libm on macOS nodejs/node#56901 * test: make eval snapshot comparison more flexible nodejs/node#57020 * src: allow embedder customization of OOMErrorHandler nodejs/node#57325 * src: do not pass nullptr to std::string ctor nodejs/node#57354 * src: lock the isolate properly in IsolateData destructor nodejs/node#57031 * chore: shrink --trace-atomics-wait patch * chore: fixup patch indices * build: fix GN build failure nodejs/node#57013 * crypto: expose security levels nodejs/node#56601 * zlib: add zstd support nodejs/node#52100 * test: move crypto related common utilities in common/crypto nodejs/node#56714 * cli: move --trace-atomics-wait to eol nodejs/node#52747 * test: disable test-https-client-renegotiation-limit BoringSSL doesn't support caller-initiated renegotiation - see https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request May 5, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
dbdefef
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 6, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
079024e
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 6, 2025
@electron-roller@codebytere
chore: bump node to v22.15.0 (37-x-y) (#46869)
cd023b5
* chore: bump node in DEPS to v22.15.0 * inspector: fix GN build nodejs/node#56798 * test: search cctest files nodejs/node#56791 * crypto: fix missing OPENSSL_NO_ENGINE guard nodejs/node#57012 * test,crypto: make tests work for BoringSSL nodejs/node#57021 * module: use synchronous hooks for preparsing in import(cjs) nodejs/node#55698 * deps: update simdjson to 3.12.0 nodejs/node#56874 * build: remove explicit linker call to libm on macOS nodejs/node#56901 * test: make eval snapshot comparison more flexible nodejs/node#57020 * src: allow embedder customization of OOMErrorHandler nodejs/node#57325 * src: do not pass nullptr to std::string ctor nodejs/node#57354 * src: lock the isolate properly in IsolateData destructor nodejs/node#57031 * chore: shrink --trace-atomics-wait patch * chore: fixup patch indices * build: fix GN build failure nodejs/node#57013 * crypto: expose security levels nodejs/node#56601 * zlib: add zstd support nodejs/node#52100 * test: move crypto related common utilities in common/crypto nodejs/node#56714 * cli: move --trace-atomics-wait to eol nodejs/node#52747 * test: disable test-https-client-renegotiation-limit BoringSSL doesn't support caller-initiated renegotiation - see https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@electron-roller@codebytere@kigh-ota
chore: bump node to v22.15.0 (main) (electron#46742)
2f58fe0
* chore: bump node in DEPS to v22.15.0 * inspector: fix GN build nodejs/node#56798 * test: search cctest files nodejs/node#56791 * crypto: fix missing OPENSSL_NO_ENGINE guard nodejs/node#57012 * test,crypto: make tests work for BoringSSL nodejs/node#57021 * module: use synchronous hooks for preparsing in import(cjs) nodejs/node#55698 * deps: update simdjson to 3.12.0 nodejs/node#56874 * build: remove explicit linker call to libm on macOS nodejs/node#56901 * test: make eval snapshot comparison more flexible nodejs/node#57020 * src: allow embedder customization of OOMErrorHandler nodejs/node#57325 * src: do not pass nullptr to std::string ctor nodejs/node#57354 * src: lock the isolate properly in IsolateData destructor nodejs/node#57031 * chore: shrink --trace-atomics-wait patch * chore: fixup patch indices * build: fix GN build failure nodejs/node#57013 * crypto: expose security levels nodejs/node#56601 * zlib: add zstd support nodejs/node#52100 * test: move crypto related common utilities in common/crypto nodejs/node#56714 * cli: move --trace-atomics-wait to eol nodejs/node#52747 * test: disable test-https-client-renegotiation-limit BoringSSL doesn't support caller-initiated renegotiation - see https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@anonriganonriganonrig approved these changes

@legendecaslegendecaslegendecas approved these changes

@RafaelGSSRafaelGSSRafaelGSS approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung@nodejs-github-bot@targos@jasnell@addaleax@anonrig@legendecas@RafaelGSS