Skip to content

Conversation

@jawshooah
Copy link
Collaborator

Move process logic from Utils.execute_in_background to Subprocess.spawn_in_background. Use ChildProcess rather than low-level Process module for future-proofing, since ChildProcess is Windows-compatible.

Edit: Per the conversation in the comments, this PR now simply extracts the Process.detach(Process.spawn(...)) logic to Subprocess.spawn_detached in the interest of separation of concerns.

Copy link
Owner

Choose a reason for hiding this comment

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

How does this deal with long-running background processes that continue running after the hook run has completed?

The reason we used Process.detach here was to ensure that we wouldn't create a zombie process. I'm not sure how ChildProcess deals with this situation, but so far it hasn't been necessary to worry about because we don't use it for these sorts of background tasks—it's always been used in a context that blocks the parent Overcommit process until it completes.

Furthermore, I'm not sure what happens to the child process when terminating the parent process (does ChildProcess auto-terminate the child for us?). We wouldn't want this to happen for hooks like IndexTags which can take many seconds to index large repositories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can detach the ChildProcess using process.detach = true before executing process.start. I only omitted that line because process.exited? throws an error when process is detached, which caused the spec to fail. Didn't consider the problem of zombie processes, and wasn't sure how to work around the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't seem like ChildProcess does anything special to terminate the created child process when the parent dies, but that should happen anyway unless the child process has been detached, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regardless, we can just use the original Process.detach(Process.spawn(...)) logic here and save the ChildProcess stuff for later, when we figure out Windows compatibility.

@jawshooah jawshooah changed the title Use ChildProcess to spawn processes in the background Extract process logic from Utils.execute_in_background into Subprocess.spawn_detached Apr 8, 2015
@sds sds added the enhancement label Apr 8, 2015
@sds
Copy link
Owner

sds commented Apr 8, 2015

Merged in 3b67c5d.

@sds sds closed this Apr 8, 2015
@jawshooah jawshooah deleted the subprocess-detach branch April 8, 2015 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants