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.

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

Ensure bash shell executions close file descriptors #3328

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

seanmil
Copy link
Contributor

@seanmil seanmil commented Jun 26, 2024

Currently Bolt::Shell::Bash will leaves open file descriptors at the conclusion of execute(). The file descriptions do fall out of scope at the conclusion of the execute() method and the Ruby GC eventually closes them. However, on a very busy Bolt invocation with lots of short-lived Tasks or a number of Tasks running in parallel (e.g. via background()) the number of open FDs before garbage collection can get moderately high, hitting problems on systems with low file descriptor limits.

In some simple testing in my environment this reduced the number of consistently open FDs from around 90-110 to somewhere in the 50s for a given test scenario.

Currently Bolt::Shell::Bash will leaves open file
descriptors at the conclusion of execute(). The
file descriptions do fall out of scope at the conclusion
of the execute() method and the Ruby GC eventually closes
them. However, on a very busy Bolt invocation with lots of
short-lived Tasks or a number of Tasks running in parallel
(e.g. via background()) the number of open FDs before
garbage collection can get moderately high, hitting problems
on systems with low file descriptor limits.

!bug

* **Explicitly close Bolt::Shell::Bash file descriptors**

  Ensure file descriptors in Bolt::Shell::Bash are explicitly
  closed, helping to alleviate the chance of hitting file
  descriptor limits on systems with low defaults (e.g. Mac OS).
@seanmil seanmil requested a review from a team as a code owner June 26, 2024 16:50
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

Wow, nice! thanks for finding this.

@donoghuc donoghuc merged commit a744d03 into puppetlabs:main Jun 26, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants