gps: cmd_unix: remove extra command context; use Cmd.Process.Kill() directly #1269
Conversation
|
@tamird care to take a look? Have I mistaken how this works or was intended? |
|
Ah, I see -- nice catch. Though perhaps now that we're not using context propagation for anything, it might be clearer and less error-prone to use Also, please be sure to include the full rationale in the commit message - it's a real PITA to have to look up the PR when digging through |
|
ugh...i also found and fixed this yesterday, but i guess i didn't push up the change. turns out, though, that this makes no difference at all in the normal case of ctrl-c sent from the CLI, because of...well, have a look at #1270, which i just put up.
yeah, we should move to this as a standard. |
internal/gps/cmd_unix.go
Outdated
| return cmd{ctx: ctx, Cmd: exec.Command(name, arg...)} | ||
| } | ||
|
|
||
| func (c cmd) kill() { |
There was a problem hiding this comment.
I don't think this helper is helpful - just inline this into two places it's referenced.
There was a problem hiding this comment.
Signature doesn't match, so an annoymous wrapper function was required. No strong preference either way though
There was a problem hiding this comment.
Oh, you mean because it returns an error? Still seems better to spell it out. Now that I look at it, the stopCancel binding isn't needed either:
...
defer time.AfterFunc(time.Minute, func() {
_ = c.Cmd.Process.Kill()
}).Stop()
|
Text wrapping in the commit message is messed up, btw. |
jmank88
changed the title
|
Rebased. |
internal/gps/cmd_unix.go
Outdated
| _ = c.Cmd.Process.Kill() | ||
| } else { | ||
| stopCancel := time.AfterFunc(time.Minute, c.cancel).Stop | ||
| // After one minute, resort to kill. |
There was a problem hiding this comment.
I'd avoid this comment; the "one minute" bit is asking for rot.
|
@sdboyer can you merge this? |
| var b bytes.Buffer | ||
| c.Cmd.Stdout = &b | ||
| c.Cmd.Stderr = &b | ||
|
|
5 participants
What does this do / why do we need it?
This PR removes an extra command context and switches to use Cmd.Process.Kill() directly, rather than indirectly triggering the kill via cancelling the context.
Follow up from #1270.