Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Made command timeouts configurable #1028

Closed
wants to merge 2 commits into from
Closed

Made command timeouts configurable #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f3f230
Made command timeouts configurable
ereOn Aug 17, 2017
12a8bc2
Got rid of the Windows mention in the timeouts section
ereOn Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,15 @@ You might, for example, include a constraint in your manifest that specifies `ve
* `0.2.3` becomes the range `>=0.2.3, <0.3.0`
* `0.0.3` becomes the range `>=0.0.3, <0.1.0`

## Timeouts

`dep ensure` can sometime fail on very slow system due to underlying operations taking too long to perform.

If the built-in timeouts are not suitable to your environment, you can override those by setting specific environment variables:

* `DEP_EXPENSIVE_CMD_TIMEOUT`: Set the maximum number of seconds expensive operations can take. The default is 2 minutes (120 seconds).
* `DEP_DEFAULT_CMD_TIMEOUT`: Set the maximum number of seconds common operations can take. The default is 10 seconds.

## Feedback

Feedback is greatly appreciated.
Expand Down
25 changes: 23 additions & 2 deletions internal/gps/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (
"bytes"
"context"
"fmt"
"os"
"os/exec"
"strconv"
"sync"
"sync/atomic"
"time"
Expand Down Expand Up @@ -196,12 +198,31 @@ func runFromRepoDir(ctx context.Context, repo vcs.Repo, timeout time.Duration, c
return c.combinedOutput(ctx)
}

func getenvDuration(key string, def time.Duration) time.Duration {
seconds, err := strconv.Atoi(os.Getenv(key))

if err != nil {
return def
Copy link
Collaborator

Choose a reason for hiding this comment

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

This gives no indication to the user when their environment variable is set but isn't convertible.

Copy link
Author

Choose a reason for hiding this comment

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

Correct. It might be confusing.

I don't mind making this more user-friendly. However I will put that on hold until we have consensus that the environment variables are indeed the way to go.

}

return time.Duration(seconds) * time.Second
}

const (
// envExpensiveCmdTimeout is the environment variable to read for the
// expensiveCmdTimeout default, in seconds.
envExpensiveCmdTimeout = "DEP_EXPENSIVE_CMD_TIMEOUT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently dep isn't relying upon any environment variables (other than in tests). I'm not sure if we want to start doing that, instead of continuing to use flags? I'll let @sdboyer have the final say on that though.

If we do stick with environment variables, they should be documented in the help text too.

Copy link
Author

@ereOn ereOn Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I actually felt uncomfortable adding those environment variables where no other were currently used.

I went with those anyway because:

  • Timeouts are not something I'd like to set on each invocation of dep. If an operation takes a long time on my machine, this is likely to happen often - or even every time.
  • It somehow felt like leaking an implementation detail: if we end up having 15 sorts of timeouts in dep and all of those have to be exposed as flags (part of the CLI API so to speak), things will get ugly fast.

Perhaps a configuration file of some sort (like git has), would serve us better in that regard.

That being said, when in Rome, I'll do as the romans do so feel free to confirm or reject my decisions and I'll happily update my PR !

dep is awesome :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good point, I hadn't thought of that! Personally I'm fine with the environment variable. Let's move forward with that for now. 👍

The next step is to get some tests that exercise changing the timeout. I don't know how easy/hard that will end up being so if you find yourself going down a rabbit hole, post back here and we can think about it more!

// envDefaultCmdTimeout is the environment variable to read for the
// envDefaultCmdTimeout default, in seconds.
envDefaultCmdTimeout = "DEP_DEFAULT_CMD_TIMEOUT"
)

var (
// expensiveCmdTimeout is meant to be used in a command that is expensive
// in terms of computation and we know it will take long or one that uses
// the network, such as clones, updates, ....
expensiveCmdTimeout = 2 * time.Minute
expensiveCmdTimeout = getenvDuration(envExpensiveCmdTimeout, 2*time.Minute)
// defaultCmdTimeout is just an umbrella value for all other commands that
// should not take much.
defaultCmdTimeout = 10 * time.Second
defaultCmdTimeout = getenvDuration(envDefaultCmdTimeout, 10*time.Second)
)