Include 'timeout' parameter in Git execute #354
Conversation
This feature enables to set a timeout while executing a git command. After this timeout is over, the process will be killed. If not explicitly specified, the default functionality will not be affected. Change-Id: I2dd5f0de7cb1f5f1b4253dd7ce92d23551d5f9a7
|
Thanks a lot for the PR ! The feature is much appreciated. When looking at the implementation, I would wonder whether one should always use the same code-path, or have two ones. One will deal with the default (and common) case where Additionally, GitPython claims to run and work on windows, which would stop being the case without additional work. To me it would be sufficient to just not provide a What do you think ? |
|
@Byron thanks for the feedback. We're working on a couple of new commits to address the comments, and will upload them next week. |
Change-Id: I2e081c606b75b7f8d3d1ee82d93c3d9f3bdcfcbe
…mand If the timeout is not specified, we don't need the overhead of creating a watchdog and event. Change-Id: I53ff891af24d4c27fb16bf4bb35910dd1d19d238
| for child_pid in child_pids: | ||
| os.kill(child_pid, SIGKILL) | ||
| kill_check.set() # tell the main routine that the process was killed | ||
| except OSError: |
There was a problem hiding this comment.
If os.kill throws OSError if the process to be killed doesn't exist, I fear that after os.kill(pid, SIGKILL), some of the child processes might already be down as pid. Right now, failure to kill the first child process, will prevent the others from being downed as well. Maybe something like that would help:
for child_pid in child_pids:
try:
os.kill(child_pid, SIGKILL)
except OSError:
pass
|
Thanks for the previous batch of changes. I hope you won't mind the additional comments I have left - just now I realized what it means to In case you make any changes, I would be glad if you drop a comment as well as this will trigger a notification, of which I receive none if additional commits are pushed into the PR. |
Change-Id: I8ab3d5affb3f040dd9630687fb20aedbd7510070
Specify that this feature is not supported on Windows and mention about the negative side-effects of SIGKILL on a repository. Change-Id: Ibba2c3f51f84084b4637ae9aaafa87dd84000ef4
Right now, we come out of the iteration in case of failure while trying to kill a child pid. This may result in some of the child pids staying alive. Change-Id: I18d58fcefec2bbdae4ae9bf73594939ade241b52
|
@Byron I just added some more commits to address your latest comments. Thanks. |
Include 'timeout' parameter in Git execute
|
Thanks a lot for your efforts ! |
This feature enables to set a timeout while executing a git
command. After this timeout is over, the process will be killed.
If not explicitly specified, the default functionality will not
be affected.