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

fix(*): add missing close on process that runs commands as bash, rename abort to close #294

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

gyuho
Copy link
Collaborator

@gyuho gyuho commented Jan 14, 2025

Fix

open /tmp/tmpbash888507873.bash: too many open files

@gyuho gyuho self-assigned this Jan 14, 2025
@xiang90
Copy link
Contributor

xiang90 commented Jan 14, 2025

any way we can test this?

@xiang90
Copy link
Contributor

xiang90 commented Jan 14, 2025

need to have unit test or end to end test to ensure there is no leak

@gyuho gyuho changed the title fix(*): add missing abort on process that runs commands as bash fix(*): add missing close on process that runs commands as bash, rename abort to close Jan 15, 2025
@gyuho gyuho force-pushed the add-missing-abort branch 2 times, most recently from 751d553 to a8001ef Compare January 17, 2025 13:06
@gyuho
Copy link
Collaborator Author

gyuho commented Jan 18, 2025

@xiang90 @cardyok unit tested and end to end tested (confirmed that no temp file leaks for the last 24 hour)

@xiang90
Copy link
Contributor

xiang90 commented Jan 18, 2025

lgtm. defer to @cardyok

…me abort to close

> open /tmp/tmpbash888507873.bash: too many open files

Signed-off-by: Gyuho Lee <[email protected]>
@gyuho gyuho force-pushed the add-missing-abort branch 2 times, most recently from f26b07b to a853195 Compare January 18, 2025 14:13
Copy link
Collaborator

@cardyok cardyok left a comment

Choose a reason for hiding this comment

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

lgtm

@gyuho gyuho added this to the v0.4.0 milestone Jan 19, 2025
@gyuho gyuho merged commit a81e4a0 into main Jan 19, 2025
8 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.

3 participants