From 044b1dd0f29c8f980f47a01a1cf547d7848a8056 Mon Sep 17 00:00:00 2001 From: Miguel Molina Date: Sat, 22 Jul 2017 00:42:24 +0200 Subject: [PATCH] use different timeouts for all vcs commands both `runFromRepoDir` and `runFromCwd` accept now a timeout to control the timeouts of the underlying `monitoredCmd`, instead of having a hardcoded timeout of 2 minutes for all commands. This will allow better tuning of the timeouts on a per-command basis. Signed-off-by: Miguel Molina --- internal/gps/cmd.go | 18 ++++++++++++++---- internal/gps/vcs_repo.go | 34 +++++++++++++++++----------------- internal/gps/vcs_source.go | 14 +++++++------- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/internal/gps/cmd.go b/internal/gps/cmd.go index 73d8702d7c..c068a53bda 100644 --- a/internal/gps/cmd.go +++ b/internal/gps/cmd.go @@ -136,12 +136,22 @@ func (e killCmdError) Error() string { return fmt.Sprintf("error killing command: %s", e.err) } -func runFromCwd(ctx context.Context, cmd string, args ...string) ([]byte, error) { - c := newMonitoredCmd(exec.Command(cmd, args...), 2*time.Minute) +func runFromCwd(ctx context.Context, timeout time.Duration, cmd string, args ...string) ([]byte, error) { + c := newMonitoredCmd(exec.Command(cmd, args...), timeout) return c.combinedOutput(ctx) } -func runFromRepoDir(ctx context.Context, repo vcs.Repo, cmd string, args ...string) ([]byte, error) { - c := newMonitoredCmd(repo.CmdFromDir(cmd, args...), 2*time.Minute) +func runFromRepoDir(ctx context.Context, repo vcs.Repo, timeout time.Duration, cmd string, args ...string) ([]byte, error) { + c := newMonitoredCmd(repo.CmdFromDir(cmd, args...), timeout) return c.combinedOutput(ctx) } + +const ( + // 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 + // defaultCmdTimeout is just an umbrella value for all other commands that + // should not take much. + defaultCmdTimeout = 10 * time.Second +) diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index fa21cf759f..f99ac9e6ca 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -80,7 +80,7 @@ func newVcsLocalErrorOr(msg string, err error, out string) error { } func (r *gitRepo) get(ctx context.Context) error { - out, err := runFromCwd(ctx, "git", "clone", "--recursive", "-v", "--progress", r.Remote(), r.LocalPath()) + out, err := runFromCwd(ctx, expensiveCmdTimeout, "git", "clone", "--recursive", "-v", "--progress", r.Remote(), r.LocalPath()) if err != nil { return newVcsRemoteErrorOr("unable to get repository", err, string(out)) } @@ -90,7 +90,7 @@ func (r *gitRepo) get(ctx context.Context) error { func (r *gitRepo) fetch(ctx context.Context) error { // Perform a fetch to make sure everything is up to date. - out, err := runFromRepoDir(ctx, r, "git", "fetch", "--tags", "--prune", r.RemoteLocation) + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "fetch", "--tags", "--prune", r.RemoteLocation) if err != nil { return newVcsRemoteErrorOr("unable to update repository", err, string(out)) } @@ -98,7 +98,7 @@ func (r *gitRepo) fetch(ctx context.Context) error { } func (r *gitRepo) updateVersion(ctx context.Context, v string) error { - out, err := runFromRepoDir(ctx, r, "git", "checkout", v) + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "checkout", v) if err != nil { return newVcsLocalErrorOr("Unable to update checked out version", err, string(out)) } @@ -110,20 +110,20 @@ func (r *gitRepo) updateVersion(ctx context.Context, v string) error { // submodules. Or nested submodules. What a great idea, submodules. func (r *gitRepo) defendAgainstSubmodules(ctx context.Context) error { // First, update them to whatever they should be, if there should happen to be any. - out, err := runFromRepoDir(ctx, r, "git", "submodule", "update", "--init", "--recursive") + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "submodule", "update", "--init", "--recursive") if err != nil { return newVcsLocalErrorOr("unexpected error while defensively updating submodules", err, string(out)) } // Now, do a special extra-aggressive clean in case changing versions caused // one or more submodules to go away. - out, err = runFromRepoDir(ctx, r, "git", "clean", "-x", "-d", "-f", "-f") + out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "clean", "-x", "-d", "-f", "-f") if err != nil { return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict submodule directories", err, string(out)) } // Then, repeat just in case there are any nested submodules that went away. - out, err = runFromRepoDir(ctx, r, "git", "submodule", "foreach", "--recursive", "git", "clean", "-x", "-d", "-f", "-f") + out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "submodule", "foreach", "--recursive", "git", "clean", "-x", "-d", "-f", "-f") if err != nil { return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict nested submodule directories", err, string(out)) } @@ -144,7 +144,7 @@ func (r *bzrRepo) get(ctx context.Context) error { } } - out, err := runFromCwd(ctx, "bzr", "branch", r.Remote(), r.LocalPath()) + out, err := runFromCwd(ctx, expensiveCmdTimeout, "bzr", "branch", r.Remote(), r.LocalPath()) if err != nil { return newVcsRemoteErrorOr("unable to get repository", err, string(out)) } @@ -153,7 +153,7 @@ func (r *bzrRepo) get(ctx context.Context) error { } func (r *bzrRepo) fetch(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, "bzr", "pull") + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "pull") if err != nil { return newVcsRemoteErrorOr("unable to update repository", err, string(out)) } @@ -161,7 +161,7 @@ func (r *bzrRepo) fetch(ctx context.Context) error { } func (r *bzrRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, "bzr", "update", "-r", version) + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "update", "-r", version) if err != nil { return newVcsLocalErrorOr("unable to update checked out version", err, string(out)) } @@ -173,7 +173,7 @@ type hgRepo struct { } func (r *hgRepo) get(ctx context.Context) error { - out, err := runFromCwd(ctx, "hg", "clone", r.Remote(), r.LocalPath()) + out, err := runFromCwd(ctx, expensiveCmdTimeout, "hg", "clone", r.Remote(), r.LocalPath()) if err != nil { return newVcsRemoteErrorOr("unable to get repository", err, string(out)) } @@ -182,7 +182,7 @@ func (r *hgRepo) get(ctx context.Context) error { } func (r *hgRepo) fetch(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, "hg", "pull") + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "pull") if err != nil { return newVcsRemoteErrorOr("unable to fetch latest changes", err, string(out)) } @@ -190,7 +190,7 @@ func (r *hgRepo) fetch(ctx context.Context) error { } func (r *hgRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, "hg", "update", version) + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "update", version) if err != nil { return newVcsRemoteErrorOr("unable to update checked out version", err, string(out)) } @@ -210,7 +210,7 @@ func (r *svnRepo) get(ctx context.Context) error { remote = "file:///" + remote } - out, err := runFromCwd(ctx, "svn", "checkout", remote, r.LocalPath()) + out, err := runFromCwd(ctx, expensiveCmdTimeout, "svn", "checkout", remote, r.LocalPath()) if err != nil { return newVcsRemoteErrorOr("unable to get repository", err, string(out)) } @@ -219,7 +219,7 @@ func (r *svnRepo) get(ctx context.Context) error { } func (r *svnRepo) update(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, "svn", "update") + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update") if err != nil { return newVcsRemoteErrorOr("unable to update repository", err, string(out)) } @@ -228,7 +228,7 @@ func (r *svnRepo) update(ctx context.Context) error { } func (r *svnRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, "svn", "update", "-r", version) + out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update", "-r", version) if err != nil { return newVcsRemoteErrorOr("unable to update checked out version", err, string(out)) } @@ -250,7 +250,7 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) { Commit commit `xml:"entry>commit"` } - out, err := runFromRepoDir(ctx, r, "svn", "info", "-r", id, "--xml") + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "info", "-r", id, "--xml") if err != nil { return nil, newVcsLocalErrorOr("unable to retrieve commit information", err, string(out)) } @@ -267,7 +267,7 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) { } } - out, err := runFromRepoDir(ctx, r, "svn", "log", "-r", id, "--xml") + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "log", "-r", id, "--xml") if err != nil { return nil, newVcsRemoteErrorOr("unable to retrieve commit information", err, string(out)) } diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index 397cfd1b32..502e620062 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -136,7 +136,7 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin // could have an err here...but it's hard to imagine how? defer fs.RenameWithFallback(bak, idx) - out, err := runFromRepoDir(ctx, r, "git", "read-tree", rev.String()) + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "read-tree", rev.String()) if err != nil { return fmt.Errorf("%s: %s", out, err) } @@ -152,7 +152,7 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin // though we have a bunch of housekeeping to do to set up, then tear // down, the sparse checkout controls, as well as restore the original // index and HEAD. - out, err = runFromRepoDir(ctx, r, "git", "checkout-index", "-a", "--prefix="+to) + out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "checkout-index", "-a", "--prefix="+to) if err != nil { return fmt.Errorf("%s: %s", out, err) } @@ -365,7 +365,7 @@ func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } // Now, list all the tags - out, err := runFromRepoDir(ctx, r, "bzr", "tags", "--show-ids", "-v") + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "tags", "--show-ids", "-v") if err != nil { return nil, fmt.Errorf("%s: %s", err, string(out)) } @@ -373,7 +373,7 @@ func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) { all := bytes.Split(bytes.TrimSpace(out), []byte("\n")) var branchrev []byte - branchrev, err = runFromRepoDir(ctx, r, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.") + branchrev, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.") br := string(branchrev) if err != nil { return nil, fmt.Errorf("%s: %s", err, br) @@ -416,7 +416,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } // Now, list all the tags - out, err := runFromRepoDir(ctx, r, "hg", "tags", "--debug", "--verbose") + out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "tags", "--debug", "--verbose") if err != nil { return nil, fmt.Errorf("%s: %s", err, string(out)) } @@ -450,7 +450,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { // bookmarks next, because the presence of the magic @ bookmark has to // determine how we handle the branches var magicAt bool - out, err = runFromRepoDir(ctx, r, "hg", "bookmarks", "--debug") + out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "bookmarks", "--debug") if err != nil { // better nothing than partial and misleading return nil, fmt.Errorf("%s: %s", err, string(out)) @@ -483,7 +483,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } } - out, err = runFromRepoDir(ctx, r, "hg", "branches", "-c", "--debug") + out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "branches", "-c", "--debug") if err != nil { // better nothing than partial and misleading return nil, fmt.Errorf("%s: %s", err, string(out))