Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Go 2: os/exec: follow Context guidelines #26422

Open
caseybarker opened this issue Jul 17, 2018 · 4 comments

Comments

@caseybarker
Copy link

commented Jul 17, 2018

Proposal: Make exec follow guidelines for using Context objects

Author(s): Casey Barker

Last updated: 2018-07-17

Abstract

This proposal is to add cmd.StartCtx() and cmd.RunCtx() methods to the exec package, eventually deprecating exec.CommandContext(), and bringing it into compliance with the guidelines for use of Context objects.

Background

The documentation for the context package says:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx.

The exec.CommandContext() function breaks this rule; it stores the ctx parameter inside the Cmd object until later referenced by the cmd.Start() method (which is also called by cmd.Run()). A caller of cmd.Start() or cmd.Run() has no control over which context gets used in the execution.

Issue originally raised here:
https://groups.google.com/forum/#!topic/golang-nuts/uvJIogNTD2c

Looking at the development history for the exec package's use of context, I suspect this inconsistency arose accidentally, as the Cmd.ctx field was used differently early in development.

Proposal

  1. Add the following two new methods to the exec package:
    func (c *Cmd) StartCtx(ctx context.Context) error
    func (c *Cmd) RunCtx(ctx context.Context) error

These two methods honor the passed-in context, rather than any context that might be attached to the Cmd.

  1. Deprecate the following function in the exec package:
    func CommandContext(ctx context.Context, name string, arg ...string) *Cmd

Once the deprecation is complete, the private ctx field could be removed from the Cmd object.

Rationale

The recommendations provided by the context package are solid; the context object should be passed at the time and place where a long-running operation is started. This makes it clear which calls are long-running, and it allows the source object (in this case, the Cmd) to be created without needing to know about the ctx that might eventually be used to control the execution.

Compatibility

Part 1 (adding two new methods) does not break compatibility, although it introduces some possible confusion in that the new methods would ignore the context provided if exec.CommandContext() were initially used to create the Cmd object.

Part 2 (deprecating the exec.CommandContext() function) is a hard break in compatibility.

Implementation

TBD. This is my first proposal and I'm not yet familiar with the Go release process, but I'm willing to provide a patch if this proposal is acceptable.

@gopherbot gopherbot added this to the Proposal milestone Jul 17, 2018

@gopherbot gopherbot added the Proposal label Jul 17, 2018

@bontibon

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

Like you say, adding those methods might cause confusion with CommandContext. I might be better just wait until Go2 where CommandContext can be removed and the signatures of Run and Start.changed to accept a context.Context.

func (c *Cmd) StartCtx(ctx *context.Context) error
func (c *Cmd) RunCtx(ctx *context.Context) error

That should not be a pointer to a context.Context.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Have you considered proposing this as a Go2 cleanup? It doesn't seem like adding functions and deprecating others will have any major advantage right now.

@mvdan mvdan changed the title Proposal: Make `exec` follow guidelines for using `Context` objects Proposal: os/exec: follow Context guidelines Jul 17, 2018

@caseybarker

This comment has been minimized.

Copy link
Author

commented Jul 17, 2018

Thanks @bontibon. I fixed the call signatures.

I agree, Go2 is the best target for this. Is there a different process for proposing Go2 changes? (Although, since the new methods don't break compatibility, they might be OK in a transitional Go 1.x release.)

@mvdan mvdan added the Go2 label Jul 17, 2018

@ianlancetaylor ianlancetaylor changed the title Proposal: os/exec: follow Context guidelines proposal: Go 2: os/exec: follow Context guidelines Jul 17, 2018

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

Also the names should be RunContext and StartContext to match the function names used elsewhere in the stdlib for context integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.