Skip to content

os: expose guessHandleType #58060

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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

os: expose guessHandleType #58060

wants to merge 4 commits into from
+79 −33

Conversation

vladfrangu
Copy link

Exposes the internal guessHandleType function, which can be used to see if a handle has a specific type, regardless of the OS it is on.

This helps out with detecting, for example, if standard input is piped into the process, instead of relying on file system calls.

Refs: #57603
@nodejs-github-bot nodejs-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. os Issues and PRs related to the os subsystem. util Issues and PRs related to the built-in util module. labels Apr 28, 2025
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
@vladfrangu
os: expose guessHandleType
da7ee62 
Exposes the internal `guessHandleType` function, which can be used to
see if a handle has a specific type, regardless of the OS it is on.

This helps out with detecting, for example, if standard input is piped
into the process, instead of relying on file system calls.

Refs: nodejs#57603
@vladfrangu
fix: validate fd in JS land + add test
302fd68
@vladfrangu
chore: fix test and add more checks
b3f78a4
@vladfrangu
chore: suggested changes
f3d0b71
@vladfrangu
Copy link
Author

I'll rebase to fix the first commit message after the CI passes! Should I also squash the PR or leave the rest as is?
@aduh95
Copy link
Contributor

aduh95 commented Apr 29, 2025

Should I also squash the PR or leave the rest as is?

As you prefer, in any case it will get squashed upon merging.
@codecov Codecov
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.13%. Comparing base (6cd1c09) to head (b88d406).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/node_util.cc 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58060      +/-   ##
==========================================
- Coverage   90.21%   90.13%   -0.08%     
==========================================
  Files         630      628       -2     
  Lines      186391   185957     -434     
  Branches    36610    36478     -132     
==========================================
- Hits       168146   167620     -526     
- Misses      11066    11223     +157     
+ Partials     7179     7114      -65
Files with missing lines Coverage Δ
lib/internal/util.js 96.22% <100.00%> (+0.12%) ⬆️
lib/os.js 98.59% <100.00%> (+<0.01%) ⬆️
src/node_util.cc 82.33% <89.47%> (+0.54%) ⬆️

... and 56 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
@vladfrangu vladfrangu force-pushed the feat/expose-guessHandleType-on-os branch from b88d406 to f3d0b71 Compare April 29, 2025 15:37
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++. needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. util Issues and PRs related to the built-in util module.
3 participants
@vladfrangu @aduh95 @nodejs-github-bot