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.

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

src,worker: add isInternalWorker #56469

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

src,worker: add isInternalWorker #56469

wants to merge 7 commits into from
+104 −4

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 commented Jan 4, 2025

This PR adds an isInternalWorker to node:worker_threads similar to isMainThread but to detect if the thread is an instance of InternalWorker

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 4, 2025
@Ceres6
Copy link
Contributor Author

Ceres6 commented Jan 4, 2025 •
edited
Loading

cc @JakobJingleheimer @bengl

@Ceres6
Copy link
Contributor Author

Ceres6 commented Jan 4, 2025

Might close nodejs/import-in-the-middle/issues/38

JakobJingleheimer
JakobJingleheimer reviewed Jan 4, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Nicely done!

Maaaybe for consistency with isMainThread, the name should actually be isInternalThread (sorry, I was the one who originally told said isInternalWorker).

doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Jan 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.52%. Comparing base (afafee2) to head (10cbd4a).
Report is 18 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56469      +/-   ##
==========================================
- Coverage   88.53%   88.52%   -0.02%     
==========================================
  Files         657      660       +3     
  Lines      190761   190898     +137     
  Branches    36616    36626      +10     
==========================================
+ Hits       168899   168996      +97     
- Misses      15048    15089      +41     
+ Partials     6814     6813       -1
Files with missing lines Coverage Δ
lib/internal/worker.js 99.81% <100.00%> (+<0.01%) ⬆️
lib/worker_threads.js 100.00% <100.00%> (ø)
src/node_worker.cc 84.15% <100.00%> (+0.20%) ⬆️
src/node_worker.h 90.90% <100.00%> (+0.90%) ⬆️

... and 43 files with indirect coverage changes

JakobJingleheimer
JakobJingleheimer reviewed Jan 5, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

nearly there! 🙌

lib/worker_threads.js Outdated Show resolved Hide resolved
lib/worker_threads.js Outdated Show resolved Hide resolved
test/parallel/test-is-internal-thread.mjs Outdated Show resolved Hide resolved
test/fixtures/worker-is-internal-thread.js Outdated Show resolved Hide resolved
test/parallel/test-is-internal-thread.mjs Outdated Show resolved Hide resolved
test/parallel/test-is-internal-thread.mjs Outdated Show resolved Hide resolved
test/parallel/test-is-internal-thread.mjs Outdated Show resolved Hide resolved
JakobJingleheimer
JakobJingleheimer approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

LGTM 😁 hopefully @bengl can confirm it does what he needs.

@daeyeon daeyeon added semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ceres6 @jasnell @JakobJingleheimer @daeyeon @nodejs-github-bot