Skip to content

[desc] Import CREATE_NEW_PROCESS_GROUP flag from subprocess#2719

Merged
fabiencastan merged 1 commit intodevelopfrom
fix/psutilFlag
May 5, 2025
Merged

[desc] Import CREATE_NEW_PROCESS_GROUP flag from subprocess#2719
fabiencastan merged 1 commit intodevelopfrom
fix/psutilFlag

Conversation

@cbentejac
Copy link
Copy Markdown
Contributor

@cbentejac cbentejac commented May 5, 2025

Description

This PR fixes an issue that occurs only on Windows when executing nodes, following #2703.

The CREATE_NEW_PROCESS_GROUP flag must be added to the list of creation flags when doing the Popen to ensure that the process to execute will not be fully detached (and hence will be killable). However, psutil does not contain any creation flags, instead supporting those from subprocess (https://psutil.readthedocs.io/en/stable/#psutil.Popen).

Prior to this PR, attempting to use psutil.CREATE_NEW_PROCESS_GROUP would cause an error on the execution for all the node processes as it did not exist. This PR replaces it by subprocess.CREATE_NEW_PROCESS_GROUP, effectively creating the process with the correct status. It is worth noting that subprocess.CREATE_NEW_PROCESS_GROUP is imported within the if sys.platform == "win32" condition as this flag only exists for Windows platforms.

@cbentejac cbentejac added this to the Meshroom 2025.1.0 milestone May 5, 2025
@cbentejac cbentejac requested review from Copilot and fabiencastan May 5, 2025 15:01
@cbentejac cbentejac self-assigned this May 5, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a Windows-specific issue by replacing the use of psutil's nonexistent creation flag with subprocess's CREATE_NEW_PROCESS_GROUP flag to ensure that spawned subprocesses can be properly terminated.

  • Import the correct flag from subprocess
  • Update the process creation arguments in the Windows branch
Comments suppressed due to low confidence (2)

meshroom/core/desc/node.py:4

  • [nitpick] The flag is now correctly imported from subprocess; please verify if psutil is still needed elsewhere in this file to avoid any unnecessary imports.
from subprocess import CREATE_NEW_PROCESS_GROUP

meshroom/core/desc/node.py:159

  • The change properly resolves the issue on Windows; please ensure through testing that this change does not inadvertently affect process group management on other platforms.
platformArgs = {"creationflags": CREATE_NEW_PROCESS_GROUP}

`psutil` does not contain these creation flags but supports
`subprocess`'s, so the one from `subprocess` needs to be used in order
for the process to be created properly.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2025

Codecov Report

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

Project coverage is 76.74%. Comparing base (006288a) to head (d096fa4).
Report is 9 commits behind head on develop.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
meshroom/core/desc/node.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2719      +/-   ##
===========================================
- Coverage    76.75%   76.74%   -0.01%     
===========================================
  Files           40       40              
  Lines         6082     6084       +2     
===========================================
+ Hits          4668     4669       +1     
- Misses        1414     1415       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fabiencastan fabiencastan merged commit 4f1e003 into develop May 5, 2025
3 of 5 checks passed
@fabiencastan fabiencastan deleted the fix/psutilFlag branch May 5, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants