Configurable job timeout #906
Conversation
| if jobTimeout != "" { | ||
| t, err := time.ParseDuration(jobTimeout) | ||
| if err != nil { | ||
| return nil, errors.New("shell: Error parsing job timeout") |
There was a problem hiding this comment.
At this point, the job has already started. If the timeout is misconfigured, any output from the job will be lost and I suspect it will be unclear to the user whether the job did or did not run.
| slowTimer := time.AfterFunc(t, func() { | ||
| j := fmt.Sprintf("shell: Job '%s' execution time exceeding defined timeout %v. Killing job.", command, t) | ||
| log.Print(j) | ||
| _, err := output.Write([]byte(j)) |
There was a problem hiding this comment.
Adding the message to the process output before it is killed, could lead to:
- output from the process being written in the output buffer after the message but before termination, which can be confusing
- the log containing a message suggesting it is killed, even though it wasn't, if killing failed for some reason.
Instead, I suggest adding a (different) message after the process has actually terminated. In this case you can also account for any truncation of the output buffer to prevent the message form being lost.
|
Thank you for a review. I've reworked a feature. Please take a look, again. Thanks! |
| return fmt.Errorf("Error parsing job timeout value") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Great! Would you mind creating a test in api_test.go for this validation, analoguous to the other validation tests?
|
|
||
| // Warn if buffer is overritten | ||
| // Warn if buffer is ovewritten |
There was a problem hiding this comment.
You made a typo correcting the typo ;D
| slowTimer := time.AfterFunc(jt, func() { | ||
| err = cmd.Process.Kill() | ||
| if err != nil { | ||
| jobTimeoutMessage = fmt.Sprintf("shell: Job '%s' execution time exceeding defined timeout %v. SIGKILL returned error. Job probably was not killed", command, jt) |
There was a problem hiding this comment.
"Job probably was not killed" - the probability of this is debatable.
Errors returned can include e.g. Process Done.
In order to prevent digging deep to find out whether it was actually killed or not (on several platforms), maybe just say "Job may not have been killed".
| err = cmd.Start() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Warn if buffer is overritten | ||
| var jobTimeoutMessage string | ||
| var jobWasKilled bool |
There was a problem hiding this comment.
The jobWasKilled value actually signifies something different. I suggest renaming it to jobTimedOut, so it is in line with jobTimeoutMessage.
|
Thanks :) here's new commit. P.S. btw, tests were failing with this one: |
Yes, that happens sometimes. It is due to randomness. |
|
Thanks @Victorcoder , but let's not forget @andreygolev 's contribution, he did the work! |
| var jobTimeoutMessage string | ||
| var jobTimedOut bool | ||
|
|
||
| slowTimer := time.AfterFunc(jt, func() { |
There was a problem hiding this comment.
I just realized that this will be 0s in case there is no timeout set. There should not be timeout when not specifying a timeout.
Could you please open a new PR with the change @andreygolev?
And yes! sorry for my fast typing, thanks for the work here @andreygolev of course!
Sometimes jobs may stuck by any reason, and we expect them to finish in a specified time.
Default behaviour is not do anything.