Skip to content

os: expose guessFileDescriptorType #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.

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

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

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
@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.

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.21%. Comparing base (a36981a) to head (f3d0b71).
Report is 12 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.21%   -0.01%     
==========================================
  Files         630      630              
  Lines      186391   186392       +1     
  Branches    36608    36619      +11     
==========================================
- Hits       168161   168156       -5     
- Misses      11052    11060       +8     
+ Partials     7178     7176       -2     
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 23 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
doc/api/os.md Outdated
added: REPLACEME
-->

* `handle` {integer} The handle number to try and guess the type of.
Copy link
Member

Choose a reason for hiding this comment

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

This should really specify that "handle" means "file descriptor". In fact, given that the fs docs almost exclusively use "file descriptor" and "fd", I would also adopt that terminology here.

Copy link
Author

Choose a reason for hiding this comment

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

Then I could also rename the function to guessFileDescriptorType? Or should I leave it as is?

And will apply the changes to fd/file descriptor in a few

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd probably do that. In the context of libuv, guessHandleType makes more sense, because it returns a value that actually corresponds to the types of libuv handles, which wrap around fds.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Let me know if I've also done the rebase correctly 🤞

@vladfrangu vladfrangu changed the title os: expose guessHandleType os: expose guessFileDescriptorType May 2, 2025
@vladfrangu vladfrangu force-pushed the feat/expose-guessHandleType-on-os branch from f3d0b71 to 0c33ea7 Compare May 2, 2025 21:18
Exposes the internal `guessHandleType` function as
`guessFileDescriptorType`, 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 vladfrangu force-pushed the feat/expose-guessHandleType-on-os branch from 0c33ea7 to 93dc673 Compare May 12, 2025 15:54
proxy->GetTarget(),
proxy->GetHandler()
};
Local<Value> ret[] = {proxy->GetTarget(), proxy->GetHandler()};
Copy link
Member

Choose a reason for hiding this comment

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

There are a large number of unrelated changes in here. Were these manually edited or did the format tool do these?

Copy link
Author

Choose a reason for hiding this comment

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

All of the formatting changes were done by saving the file (after configuring vscode with the settings example provided by the contributing guide). Not sure exactly why so many changes happened, but running the cpp formatter through make also didn't revert it. I can manually revert it if desired

@vladfrangu vladfrangu requested a review from jasnell May 16, 2025 07:44
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants