This repository was archived by the owner on Sep 9, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1k
Break context chain and move subprocesses into separate pgroups #1270
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
same comment I left in #1269; we should just not use a context here and just
c.Cmd.Process.Killdirectly instead.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 think that's racy, unfortunately. IIRC, the code that your original PR got rid of in favor of
exec.CommandContext()had all these nasty workarounds, including passing around integer address pointers for atomic ops, in order to be able to safely callc.Cmd.Process.Kill()directly without any possibility of that call being made on a nil pointer.i'll go back and double-check, but i know that the thing i really liked about using
exec.CommandContextin the first place was that it gave us a safe way of invokingKill()by taking advantage of timing information that is otherwise only accessible from within theos/execpackage.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.
Please do double check - as far as I know, there's no possibility of
Cmd.Processbeing nil afterStartreturns successfully.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.
Ah, so, it was a bit more complicated than that - while i think you're correct that there's no possibility of
Command.Processbeing nil, it wasCommand.ProcessStatethat gave problems:dep/internal/gps/cmd_unix.go
Lines 29 to 53 in e74b613
the goal there was avoiding sending a signal to a process that had already terminated, and that's the thing that's difficult to ascertain from anywhere other than the thread that's
Wait()ing. all i can remember about why i went to those lengths is because i wanted to avoid returning an error to the broader system when that error was simply a case of our mismanagement of the process (signal-after-termination), rather than an actual error from the process. yes, we could sniff the error, but...well, it's an unexported, potentially os-specific error there. not ideal.(and yes, that implies that this system should also probably be returning the cancelation error, rather than one from the underlying process, when cancelation is instructed)
so yeah, i'd prefer we stick with the reuse of context for the kill semantics here, as it both insulates us from having to think about any of the above, and may also provide a more useful point for understanding the causal ordering of events here (signal send start, signal send complete, process terminated, etc.)
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 don't understand what you're saying here. The code in
os/execis no more resilient to any of these problems; it just ignores the error returned fromProcess.Kill.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.
¯\_(ツ)_/¯ - maybe it's all equivalent. i don't have more time i can afford to invest in this right now, and figuring out if there was something else that motivated the original code. so - if you and @jmank88 can agree that just calling
Process.Kill()is equivalent, then i'll relent, and we can just use that.if not, this route feels less dragon-y to me, and without adequate time to dispel the fog, i've gotta follow my gut.
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 think I'm with @tamird here. We could do this in another PR though, right? I can put something up after this is merged for us to stare at some more in isolation.
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.
yeah, good point, can do it in a follow-up.
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 thought that's what #1269 is.