plumb context.Context everywhere; limit concurrent sub-processes with DEPMAXSUBPROCS #1695

jmank88 · 2018-02-17T16:30:55Z

What does this do / why do we need it?

This PR (1) plumbs context.Context everywhere as a prerequisite for (2) limiting concurrent sub-processes. When the env var DEPMAXSUBPROCS is set (positive integer), a semaphore is attached to the context which is picked up and aquired/released from inside cmd.CombinedOutput.

I went back and forth a bit about whether this is an appropriate use of context.Context, but we wanted to plumb it around more anyways (#423), so it won't be wasted even if we go another direction. Other considered solutions for passing the semaphore around: global variable (yuck); attaching to supervisor and using from do() (too high level) or passing it along to each vcs struct (complicated/messy).

This limits all of our get/fetch/update vcs calls, but not direct use of masterminds/vcs.Repo methods, since they don't go through our gps.commandContext(). I don't see a clean way to limit those without going too high level and also limiting method calls which e.g. only look up a struct field.

I got nearly all of the context.TODO()s, except for:

svnRepo.CommitInfo(): Overrides a method who's signature we don't control.
gps.NewSourceManager: Creates a context for supervisor. Perhaps SourceManagerConfig could have a context param (optional?), but this is a non-traditional use of Context which doesn't affect our cmd semaphore anyhow.

Do you need help or clarification on anything?

Any bright ideas about how to test this? Measuring the timing of sets of sleep commands under different limits seems sloppy.

Is DEPMAXSUBPROCS an appropriate name?

Where should I document this?

Which issue(s) does this PR fix?

#423, #1689

TODO

Document
Changelog
Tests

I pre-apologize for sending everyone to rebase hell.

jmank88 · 2018-02-17T16:39:44Z

gps/cmd.go

+
+// CtxWithCmdLimit returns a copy ctx with a semaphore value limiting the number
+// of concurrent `cmd`s. Returns an error if n is not positive.
+func CtxWithCmdLimit(ctx context.Context, n int) (context.Context, error) {


This is available for anyone using package gps, dep, or cmd/dep as a library to modify their Context.

jmank88 · 2018-02-21T16:04:51Z

We should probably think about whether we want this limit to apply to each source manager, or each function or method invocation.

Right now the caller decides, by sharing contexts as much or as little as they'd like.

If we only want to limit per source manager, then this PR can become much simpler and less consequential, with fewer exported API changes, and no external context (would still be simplest to use context internally for now, but could change) - just a new optional field on SourceManagerConfig.

danielbprice · 2018-03-28T04:17:56Z

I'm still hoping to see this change hit master. It has worked well for me in my testing. Simultaneously not having this has been causing my coworkers pain.

sdboyer · 2018-06-05T02:09:24Z

i'm picking this up, as part of the performance blitz.

sdboyer · 2018-07-04T05:26:39Z

Gonna defer this one again for a bit - i think the need is less pressing than some other things. (sorry, ARM folks)

… DEPMAXSUBPROCS

jmank88 · 2019-03-12T02:38:49Z

Rebased

kevinburke · 2019-03-12T03:35:16Z

analyzer.go

@@ -24,7 +25,7 @@ func (a Analyzer) HasDepMetadata(path string) bool {

 // DeriveManifestAndLock reads and returns the manifest at path/ManifestName or nil if one is not found.
 // The Lock is always nil for now.
-func (a Analyzer) DeriveManifestAndLock(path string, n gps.ProjectRoot) (gps.Manifest, gps.Lock, error) {
+func (a Analyzer) DeriveManifestAndLock(ctx context.Context, path string, n gps.ProjectRoot) (gps.Manifest, gps.Lock, error) {
 	if !a.HasDepMetadata(path) {
 		return nil, nil, nil
 	}


should we do f.SetDeadline(ctx.Deadline()) here ?

Yes? But I haven't addressed timeouts or cancellation in this PR, so maybe in a follow up covering those.

kevinburke · 2019-03-12T03:35:41Z

cmd/dep/check.go

@@ -58,18 +59,18 @@ func (cmd *checkCommand) Register(fs *flag.FlagSet) {
 	fs.BoolVar(&cmd.quiet, "q", false, "Suppress non-error output")
 }

-func (cmd *checkCommand) Run(ctx *dep.Ctx, args []string) error {
-	logger := ctx.Out
+func (cmd *checkCommand) Run(ctx context.Context, depCtx *dep.Ctx, args []string) error {


What's the difference between ctx and depCtx here?

or I guess, they have different API's, but why is the depCtx insufficient here? it doesn't seem like we are using ctx at all.

The checkCommand does not use the context.Context, but it must implement the command interface.

kevinburke · 2019-03-12T03:50:14Z

Is it not possible to add this behavior to dep.Ctx, which seems to already exist in a bunch of places?

One way to test would be to have a function hook before whatever logic is gating new subprocess creation. By default the hook is nil. In the test, edit the function hook to be non-nil and record whatever information you need.

jmank88 · 2019-03-12T04:05:17Z

Is it not possible to add this behavior to dep.Ctx, which seems to already exist in a bunch of places?

dep.Ctx holds high-level user configuration options. It only exists in a few high level functions. I would be in favor of renaming it to something like Config.

mvdan · 2020-09-04T15:08:58Z

Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks!

jmank88 requested review from carolynvs, darkowlzz, ibrasho and sdboyer as code owners February 17, 2018 16:30

googlebot added the cla: yes label Feb 17, 2018

jmank88 commented Feb 17, 2018

View reviewed changes

sdboyer self-assigned this Jun 5, 2018

sdboyer closed this Jun 5, 2018

sdboyer reopened this Jun 5, 2018

plumb context.Context everywhere; limit concurrent sub-processes with…

3a047cf

… DEPMAXSUBPROCS

kevinburke reviewed Mar 12, 2019

View reviewed changes

mvdan closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plumb context.Context everywhere; limit concurrent sub-processes with DEPMAXSUBPROCS #1695

plumb context.Context everywhere; limit concurrent sub-processes with DEPMAXSUBPROCS #1695

jmank88 commented Feb 17, 2018 •

edited

Loading

jmank88 Feb 17, 2018 •

edited

Loading

jmank88 commented Feb 21, 2018

danielbprice commented Mar 28, 2018

sdboyer commented Jun 5, 2018

sdboyer commented Jul 4, 2018

jmank88 commented Mar 12, 2019

kevinburke Mar 12, 2019

jmank88 Mar 12, 2019

kevinburke Mar 12, 2019

kevinburke Mar 12, 2019

jmank88 Mar 12, 2019

kevinburke commented Mar 12, 2019

jmank88 commented Mar 12, 2019 •

edited

Loading

mvdan commented Sep 4, 2020

plumb context.Context everywhere; limit concurrent sub-processes with DEPMAXSUBPROCS #1695

plumb context.Context everywhere; limit concurrent sub-processes with DEPMAXSUBPROCS #1695

Conversation

jmank88 commented Feb 17, 2018 • edited Loading

What does this do / why do we need it?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

TODO

jmank88 Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

jmank88 commented Feb 21, 2018

danielbprice commented Mar 28, 2018

sdboyer commented Jun 5, 2018

sdboyer commented Jul 4, 2018

jmank88 commented Mar 12, 2019

kevinburke Mar 12, 2019

Choose a reason for hiding this comment

jmank88 Mar 12, 2019

Choose a reason for hiding this comment

kevinburke Mar 12, 2019

Choose a reason for hiding this comment

kevinburke Mar 12, 2019

Choose a reason for hiding this comment

jmank88 Mar 12, 2019

Choose a reason for hiding this comment

kevinburke commented Mar 12, 2019

jmank88 commented Mar 12, 2019 • edited Loading

mvdan commented Sep 4, 2020

jmank88 commented Feb 17, 2018 •

edited

Loading

jmank88 Feb 17, 2018 •

edited

Loading

jmank88 commented Mar 12, 2019 •

edited

Loading