-
Notifications
You must be signed in to change notification settings - Fork 239
defect/FOUR-17640: Script execution is now halted when timeout is reached #7867
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
base: release-2025-spring
Are you sure you want to change the base?
Conversation
|
| return $timeout > 0 ? | ||
| config('app.processmaker_scripts_timeout') . " -s 9 $timeout " . config('app.processmaker_scripts_docker') : | ||
| config('app.processmaker_scripts_timeout') . " -k 1s $timeout " . config('app.processmaker_scripts_docker') : | ||
| config('app.processmaker_scripts_docker'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Changing -s 9 to -k 1s: Key Considerations"
Thank you for the change. Using -k 1s instead of -s 9 prioritizes an orderly shutdown, but there are some important points to verify to avoid potential issues:
-
Process Compatibility with
SIGTERM:- Ensure the process being managed is designed to handle signals like
SIGTERM. If the process ignores or doesn’t handle this signal properly, the change might introduce unnecessary delays without practical benefit.
- Ensure the process being managed is designed to handle signals like
-
Appropriate Grace Period:
- The 1-second (
1s) grace period might be insufficient or excessive depending on the process load or logic. Consider validating or adjusting this time based on real-world scenarios.
- The 1-second (
-
Impact on Critical Processes:
- If the context requires immediate termination due to critical risks (e.g., resource locks or high availability), switching to
-k 1smight not be appropriate.
- If the context requires immediate termination due to critical risks (e.g., resource locks or high availability), switching to
Suggestion:
Test the behavior to ensure that using -k 1s meets functional requirements and doesn’t introduce risks like prolonged delays or unexpected blockages. If it’s not feasible to guarantee proper SIGTERM handling, it might be safer to stick with -s 9.
Thank you for ensuring this change is robust!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand
gproly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved




Tickets Solved by this PR:
Solution
How to Test
docker psmultiple times in the newly-opened terminal to verify that the execution is halted.Code Review Checklist