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

Proposed changest to add a timeout to run_shell_cmd #4665

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Crivella
Copy link
Contributor

@Crivella Crivella commented Oct 1, 2024

This changes would implement the capability for run_shell_cmd to fail after a user-specified timeout.
This would be useful for commands that are known to possibly hang to fail gracefully and report meaningful logs, instead of blocking EB indefinitely.

The PR leverages the already implemented timeout in Popen.communicate (since python 3.3) for non-interactive/streamed usage of run_shell_cmd

In case of an interactive/streamed run the following has been added:

  • 2 functions (+1 helper) in run.py
    • read_pipe: read from a pipe using a thread and raise a TimeoutError in case the read operation is taking longer than the specified timeout
    • terminate_process: attempt to terminate a process gracefully by using Popen.terminate first and Popen.kill after. Raises an EasybuildError if the process is still alive after timeout
  • 3 checks inside run_shell_cmd
    • Check at the beginning of the while loop to see if the process has been alive for more than timeout time
    • Check when reading the stdout to ensure the operation does not block for more than what would make the process time exceed timeout
    • Check when reading the stderr to ensure the operation does not block for more than what would make the process time exceed timeout
  • 2 tests to ensure that both usages succeds/fails when expected.

@Micket Micket added the EasyBuild-5.0 EasyBuild 5.0 label Oct 2, 2024
@boegel boegel added this to the 5.0 milestone Oct 2, 2024
@boegel
Copy link
Member

boegel commented Mar 3, 2025

@Crivella This will need some love, due to the changes that were made in #4755

I consider this a nice-to-have for EasyBuild 5.0 since I'm not aware of any urgent need for this (though I agree it would be nice to support this), so I won't let this block the release of EasyBuild v5.0.0

@Crivella Crivella force-pushed the feature-run_shell_cmd_timeout branch from ebba4d8 to 85f5e44 Compare March 4, 2025 15:50
@Crivella
Copy link
Contributor Author

Crivella commented Mar 4, 2025

I've rebased this on top of the latest 5.0.x and adapted the code to use the changes introduced by #4755

@boegel boegel modified the milestones: 5.0.0, 5.x Mar 17, 2025
@boegel boegel changed the base branch from 5.0.x to develop March 19, 2025 10:55
@boegel boegel modified the milestones: 4.x, 5.x Mar 19, 2025
@boegel
Copy link
Member

boegel commented Mar 19, 2025

@Crivella I changed to target branch in this PR from 5.0.x to develop, you should synchronize your PR branch with current develop branch (which has received a massive update after the release of EasyBuild v5.0.0, see #4825)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

3 participants