Skip to content

src: use RAII for uv_process_options_t #59945

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
Closed
wants to merge 2 commits into from
Closed

src: use RAII for uv_process_options_t #59945

wants to merge 2 commits into from

Conversation

iknoom
Copy link
Contributor

@iknoom iknoom commented Sep 20, 2025 •
edited
Loading

This PR refactors memory management around uv_process_options_t, which is passed to uv_spawn(). It replaces manual new[]/delete[] allocations with RAII mechanisms, improving memory safety.

Previously, ProcessWrap::Spawn() could leak memory if it returned before reaching the corresponding delete[] calls. This change ensures that resources are always released automatically.

@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 Sep 20, 2025
@iknoom iknoom force-pushed the remove-malloc-with-raii branch from 410f6c2 to 13fe1b5 Compare September 20, 2025 11:52
@iknoom iknoom force-pushed the remove-malloc-with-raii branch from 13fe1b5 to 0572379 Compare September 20, 2025 11:55
@codecov Codecov
Copy link

codecov bot commented Sep 20, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 83.33333% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.44%. Comparing base (4612c79) to head (985fed4).
⚠️ Report is 46 commits behind head on main.

Files with missing lines Patch % Lines
src/process_wrap.cc 82.05% 0 Missing and 7 partials ⚠️
src/spawn_sync.cc 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59945      +/-   ##
==========================================
+ Coverage   88.25%   88.44%   +0.18%     
==========================================
  Files         703      703              
  Lines      207412   207421       +9     
  Branches    39893    39988      +95     
==========================================
+ Hits       183052   183451     +399     
+ Misses      16313    15939     -374     
+ Partials     8047     8031      -16
Files with missing lines Coverage Δ
src/spawn_sync.h 100.00% <ø> (ø)
src/spawn_sync.cc 68.92% <86.66%> (+0.24%) ⬆️
src/process_wrap.cc 69.67% <82.05%> (+3.00%) ⬆️

... and 42 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.

addaleax
addaleax reviewed Sep 21, 2025
View reviewed changes
@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 22, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 22, 2025 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/69326/ (💛)

@daeyeon daeyeon 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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 28, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in cff138c...c6316f9

nodejs-github-bot pushed a commit that referenced this pull request Sep 28, 2025
@iknoom @nodejs-github-bot
src: use RAII for uv_process_options_t
3e1adce 
PR-URL: #59945
Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs-github-bot pushed a commit that referenced this pull request Sep 28, 2025
@iknoom @nodejs-github-bot
src: fill options_args, options_env after vectors are finalized
c6316f9 
PR-URL: #59945
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax 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++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iknoom @nodejs-github-bot @addaleax @daeyeon