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

fix(status): concurrent BasicStatus creation #1135

Merged
merged 8 commits into from
Sep 21, 2017

Conversation

darkowlzz
Copy link
Collaborator

@darkowlzz darkowlzz commented Sep 7, 2017

This change adds concurrent BasicStatus creation, maintaining the order
of status output.

What does this do / why do we need it?

It reduces the time taken by status.
Before:

real    0m16.092s
user    0m0.408s
sys    0m0.394s

After:

real    0m1.777s
user    0m0.411s
sys    0m0.339s

What should your reviewer look out for in this PR?

Correctness of concurrency code.

Do you need help or clarification on anything?

Some help with the detected race condition.

Which issue(s) does this PR fix?

Related to #1008 , making status more verbose on failures.

@darkowlzz darkowlzz changed the title fix(status): concurrent BasicStatus creation [WIP] fix(status): concurrent BasicStatus creation Sep 7, 2017
@jmank88
Copy link
Collaborator

jmank88 commented Sep 7, 2017

Looks like you sorted out the concurrency.
Regarding error handling, would it still be worthwhile to output as much as possible in the event of an error, rather than logging errors and suppressing results? The work is still being done either way, and partial success can still be informative.

@darkowlzz darkowlzz changed the title [WIP] fix(status): concurrent BasicStatus creation fix(status): concurrent BasicStatus creation Sep 7, 2017
@darkowlzz
Copy link
Collaborator Author

Regarding error handling, would it still be worthwhile to output as much as possible in the event of an error, rather than logging errors and suppressing results? The work is still being done either way, and partial success can still be informative.

Yeah, that sounds reasonable.

for err := range errorCh {
ctx.Err.Println(err.Error())
}
ctx.Err.Println()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't really consider if it should still return an error. Could this look like a false positive if a user overlooks the logged errors? Should we return an error, and adjust the caller's error handling to print the buffer followed by a clear error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even that would work. Like conditionally print the out buffer (with incomplete info) depending on the error type. Although, I'm still not sure if that's the best behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, some errors are more severe than others.

What if errored items were still displayed inline, but indicated error somehow? (probably in addition to separate logs, since we don't want to cram messages into the table). Then there would be no mistaking the tail of the output for a false positive, with the errors interleaved.

Or perhaps simplest is best, what if the error output just came last?

@darkowlzz
Copy link
Collaborator Author

darkowlzz commented Sep 9, 2017

What if errored items were still displayed inline, but indicated error somehow? (probably in addition to separate logs, since we don't want to cram messages into the table). Then there would be no mistaking the tail of the output for a false positive, with the errors interleaved.
Or perhaps simplest is best, what if the error output just came last?

@jmank88 So I thought about it and even prototyped the idea.

Right now, if status is run offline, none of the network errors are logged. Or none of the errors are logged at all. I think it would be good to log the errors in verbose mode and let the user know that something failed in non-verbose mode.

How about running status offline and getting this?

$ dep status
PROJECT                             CONSTRAINT                                 VERSION                                    REVISION  LATEST   PKGS USED
github.com/Masterminds/semver       branch parse-constraints-with-dash-in-pre  branch parse-constraints-with-dash-in-pre  a93e51b   unknown  1
github.com/Masterminds/vcs          ^1.11.0                                    v1.11.1                                    3084677   unknown  1
github.com/armon/go-radix           *                                          branch master                              4239b77   unknown  1
github.com/boltdb/bolt              ^1.0.0                                     v1.3.1                                     2f1ce7a   unknown  1
github.com/go-yaml/yaml             branch v2                                  branch v2                                  cd8b52f   unknown  1
github.com/nightlyone/lockfile      *                                          branch master                              e83dc5e   unknown  1
github.com/pelletier/go-buffruneio  *                                          v0.2.0                                     c37440a   unknown  1
github.com/pelletier/go-toml        branch master                              branch master                              fe206ef   unknown  1
github.com/pkg/errors               ^0.8.0                                     v0.8.0                                     645ef00   unknown  1
github.com/sdboyer/constext         *                                          branch master                              836a144   unknown  1
golang.org/x/sys                    *                                          branch master                              bb24a47   unknown  1

Failed to get status of some projects. Run `dep status -v` to see the error messages.
failed to fetch updates

NOTE: the LATEST field is marked unknown, which is like an inline indication that something wrong happened. And a message at the bottom about how to view the errors.

I'll push the commit with these changes. Have a look, or even try it 🙂

e70cafa

@jmank88
Copy link
Collaborator

jmank88 commented Sep 9, 2017

I really like this in principle.

Could we link the final message with the unknown tag little more directly? Something like:

The statuses of %d projects are unknown due to errors. Rerun with `-v` to see details

@darkowlzz
Copy link
Collaborator Author

So I added the unknown count too.

The status of 2 projects are unknown due to errors. Rerun with `-v` flag to see details.
failed to fetch updates

And created 2 different error channels for different error types. Error due to failure in listing packages (errListPkgCh) and error due to failure in listing versions(errListVerCh).

Added 3 errors errFailedUpdate, errFailedListPkg and errMultipleFailures. Only errFailedUpdate would print the result with unknown data.

statusError = errFailedUpdate
}

errorCount = len(errListVerCh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this include len(errListPkgCh) as well?
Can probably just set it at initilization to len(errListVerCh) + len(errListPkgCh), or even just inline that expression down below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ListPackages errors are less likely to happen(no network). And even if they do, it only affects Children of a BasicStatus only for dotOutput. And for such errors, we don't show the result with unknowns. We just fail.

Error count is only for the unknown status. This should be documented properly, which errors are counted.

statusError = errMultipleFailures
} else {
statusError = errFailedUpdate
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this instead:

if statusError == nil {
  statusError = errFailedUpdate
} else {
  statuError = errMultipleFailures
}

count int
}

func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (bool, bool, errorStatus) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the errorStatus type is worth it. The return lines are sort of complex. Did you consider adding a return value instead? (bool, bool, int, error)

In either case, this function could probably use some documentation regarding what is returned. Actually, a case could be made for naming them. They are all declared in highly visible scope already anyways (bools wouldn't change), plus returns would be simpler.
Maybe (digestMismatch bool, hasMissingPkgs bool, errCnt int, err error) or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought of returning the error count, but then thought it would look weird. Returning error count when there's no error. If you think it's fine, we can return the count 😊

Yes, the return needs documentation. I'll add those variable names for the existing variables, will add error count later if we agree that we need it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought of returning the error count, but then thought it would look weird. Returning error count when there's no error. If you think it's fine, we can return the count 😊

I don't think there is a problem with this in principle, as long as the documentation makes the purpose clear. The default assumption tends to be that a non-nil err means any other return values are irrelevant, but io.Writer, for example, always returns the bytes written.

}

// While the network churns on ListVersions() requests, statically analyze
// code from the current project.
ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot))
if err != nil {
return digestMismatch, hasMissingPkgs, errors.Wrapf(err, "analysis of local packages failed")
errStatus.err = errors.Wrapf(err, "analysis of local packages failed")
return digestMismatch, hasMissingPkgs, errStatus
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the named return vars, you can just do a bare return when the values are already set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, but I think it's better to do explicit returns when the function is big, for clarity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, can we change most of these to return constant true or false for both vars? It looks like only the final hasMissingPkgs is actually dynamic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed and pushed!

Copy link
Collaborator

@jmank88 jmank88 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few optional nits

@@ -482,7 +596,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana
ctx.Err.Printf("\t%s: %s\n", fail.ex, fail.err.Error())
}

return digestMismatch, hasMissingPkgs, errors.New("address issues with undeducible import paths to get more status information")
return digestMismatch, hasMissingPkgs, 0, errors.New("address issues with undeducible import paths to get more status information")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one could be true, false, 0...

@@ -502,7 +616,7 @@ outer:
}
out.MissingFooter()

return digestMismatch, hasMissingPkgs, nil
return digestMismatch, hasMissingPkgs, 0, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one could be true, hasMissingPkgs...

And then line 566 can go away too.

digestMismatch, hasMissingPkgs, err := runStatusAll(ctx, out, p, sm)
if err != nil {
return err
digestMismatch, hasMissingPkgs, errCount, errStatus := runStatusAll(ctx, out, p, sm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe s/errStatus/err?

func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (bool, bool, error) {
var digestMismatch, hasMissingPkgs bool

func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (digestMismatch bool, hasMissingPkgs bool, errCount int, errStatus error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/errStatus/err here too?

@darkowlzz
Copy link
Collaborator Author

@jmank88 thanks for spending so much time on this 😊

out.BasicFooter()

return digestMismatch, hasMissingPkgs, nil
return false, false, errCount, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be false, false, 0, nil?

Everything else looks good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

err... no, I don't think so. That's the only place where we are counting and returning error, for partial results.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops thought I was looking at the final return.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

approving, though there are some nits - please do fix those before merging 😄

@@ -42,6 +43,17 @@ print an extended status output for each dependency of the project.
Status returns exit code zero if all dependencies are in a "good state".
`

const (
shortRev = iota
longRev = iota
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't need second iota

@@ -42,6 +43,17 @@ print an extended status output for each dependency of the project.
Status returns exit code zero if all dependencies are in a "good state".
`

const (
shortRev = iota
Copy link
Member

Choose a reason for hiding this comment

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

prefer to include a type on iota assignments, for clarity/less magic

case *dotOutput:
ptr, err := sm.ListPackages(proj.Ident(), proj.Version())
go func(proj gps.LockedProject) {
defer wg.Done()
Copy link
Member

Choose a reason for hiding this comment

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

i don't see multiple returns in this func, so no real need for the defer - just put it at the bottom.

bs.Children = prm.FlattenFn(paths.IsStandardImportPath)
}

// Split apart the version from the lock into its constituent parts
Copy link
Member

Choose a reason for hiding this comment

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

nit: complete sentences (that have periods) are preferred, so add a period - yes, i realize this is my original comment 😄

This change adds concurrent BasicStatus creation, maintaining the order
of status output.
Adds options to specify shortRev or longRev to getConsolidatedLatest().
Return explicit values instead of variables in runStatusAll().
@darkowlzz darkowlzz merged commit 88b7ad9 into golang:master Sep 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants