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.

Sign up for GitHub

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

Jump to bottom

150 parallel pull #352

Closed
wants to merge 7 commits into from
Closed

150 parallel pull #352

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1793117
Parallelized PullImage()
Apr 8, 2013
e520b04
Added better error handling
Apr 8, 2013
b4843ae
Removed FIXME
Apr 8, 2013
f9f9520
Fix for possible race condition
Apr 8, 2013
379df01
Gofmt'd
Apr 8, 2013
0773ac0
Close the id channel once filled
Apr 8, 2013
e3ebe6b
Change to error handling
Apr 9, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 38 additions & 8 deletions registry.go
View file
Open in desktop
Expand Up @@ -144,19 +144,49 @@ func (graph *Graph) PullImage(stdout io.Writer, imgId string, authConfig *auth.A
return err
}
// FIXME: Try to stream the images?
// FIXME: Lunch the getRemoteImage() in goroutines

idChan := make(chan string, len(history))

for _, j := range history {
if !graph.Exists(j.Id) {
img, layer, err := graph.getRemoteImage(stdout, j.Id, authConfig)
if err != nil {
// FIXME: Keep goging in case of error?
return err
if !graph.Exists(j.Id){
idChan <- j.Id
}
}

expectedResponseCount := len(idChan)
errChan := make(chan error)
maxWorkers := 4

for i := 0; i < maxWorkers; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of limiting the number of workers to 4? Spawning a whole bunch of goroutines is fine. Maybe you're trying to limit the number of concurrent HTTP requests?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it's to limit the number of concurrent requests.

go func () {
for len(idChan) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is racy. Another worker could pop the last item off the channel in the meantime. The fix is to do something like

go func() {
  for id := range c {
    img, layer, err := graph.getRemoteImage(stdout, <-idChan, authConfig)
    // ...
  }
}()

img, layer, err := graph.getRemoteImage(stdout, <-idChan, authConfig)
if err != nil {
errChan <- err
continue
}
if err = graph.Register(layer, img); err != nil {
errChan <- err
continue
}
errChan <- nil
}
if err = graph.Register(layer, img); err != nil {
return err
}()
}

var errMsg error

for i := 0; i < expectedResponseCount; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you range over errChan instead of doing this loop? (And then you can get rid of expectedResponseCount.)

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. I'd have to close the channel from one of the goroutines, and I can't tell if another is still in progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I didn't read this closely enough. My mistake.

if err = <- errChan; err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure you've gofmted your changes (you can use make fmt).

if errMsg == nil {
errMsg = fmt.Errorf("The following errors were encountered while downloading images:\n")
}
errMsg = fmt.Errorf("%s\n%s",errMsg, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a simple []string and strings.Join would be nicer here? You wouldn't allocate as many new strings, you wouldn't have the overhead of reflection and the code would be more straightforward to understand, by not having the "is it initialized yet?" check on every iteration.

errors := []string{"The following errors..."}
for ... {
  // append if error
}

if len(errors) > 1 {
  return errors.New(strings.Join(errors, "\n"))
}

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Updated

}
if errMsg != nil {
return errMsg
}
return nil
}

Expand Down