Merge pull request #21399 from LK4D4/fix_builder_race

builder: replace cancelled channel with net/context

api/server/router/build/backend.go

@@ -16,5 +16,5 @@ type Backend interface {
 	// by the caller.
 	//
 	// TODO: make this return a reference instead of string
-	Build(clientCtx context.Context, config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer, clientGone <-chan bool) (string, error)
+	Build(clientCtx context.Context, config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer) (string, error)
 }

api/server/router/build/build_routes.go

@@ -161,17 +161,12 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 		return progress.NewProgressReader(in, progressOutput, r.ContentLength, "Downloading context", remoteURL)
 	}
 
-	var (
+	buildContext, dockerfileName, err := builder.DetectContextFromRemoteURL(r.Body, remoteURL, createProgressReader)
-		context        builder.ModifiableContext
-		dockerfileName string
-		out            io.Writer
-	)
-	context, dockerfileName, err = builder.DetectContextFromRemoteURL(r.Body, remoteURL, createProgressReader)
 	if err != nil {
 		return errf(err)
 	}
 	defer func() {
-		if err := context.Close(); err != nil {
+		if err := buildContext.Close(); err != nil {
 			logrus.Debugf("[BUILDER] failed to remove temporary context: %v", err)
 		}
 	}()
@@ -181,7 +176,7 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 
 	buildOptions.AuthConfigs = authConfigs
 
-	out = output
+	var out io.Writer = output
 	if buildOptions.SuppressOutput {
 		out = notVerboseBuffer
 	}
@@ -189,15 +184,24 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 	stdout := &streamformatter.StdoutFormatter{Writer: out, StreamFormatter: sf}
 	stderr := &streamformatter.StderrFormatter{Writer: out, StreamFormatter: sf}
 
-	closeNotifier := make(<-chan bool)
+	finished := make(chan struct{})
+	defer close(finished)
 	if notifier, ok := w.(http.CloseNotifier); ok {
-		closeNotifier = notifier.CloseNotify()
+		notifyContext, cancel := context.WithCancel(ctx)
+		closeNotifier := notifier.CloseNotify()
+		go func() {
+			select {
+			case <-closeNotifier:
+				cancel()
+			case <-finished:
+			}
+		}()
+		ctx = notifyContext
 	}
 
 	imgID, err := br.backend.Build(ctx, buildOptions,
-		builder.DockerIgnoreContext{ModifiableContext: context},
+		builder.DockerIgnoreContext{ModifiableContext: buildContext},
-		stdout, stderr, out,
+		stdout, stderr, out)
-		closeNotifier)
 	if err != nil {
 		return errf(err)
 	}

builder/dockerfile/builder.go

@@ -8,7 +8,6 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
-	"sync"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/builder"
@@ -56,6 +55,7 @@ type Builder struct {
 	docker    builder.Backend
 	context   builder.Context
 	clientCtx context.Context
+	cancel    context.CancelFunc
 
 	dockerfile       *parser.Node
 	runConfig        *container.Config // runconfig for cmd, run, entrypoint etc.
@@ -67,8 +67,6 @@ type Builder struct {
 	cmdSet           bool
 	disableCommit    bool
 	cacheBusted      bool
-	cancelled        chan struct{}
-	cancelOnce       sync.Once
 	allowedBuildArgs map[string]bool // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.
 
 	// TODO: remove once docker.Commit can receive a tag
@@ -88,23 +86,24 @@ func NewBuildManager(b builder.Backend) (bm *BuildManager) {
 // NewBuilder creates a new Dockerfile builder from an optional dockerfile and a Config.
 // If dockerfile is nil, the Dockerfile specified by Config.DockerfileName,
 // will be read from the Context passed to Build().
-func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, context builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) {
+func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, buildContext builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) {
 	if config == nil {
 		config = new(types.ImageBuildOptions)
 	}
 	if config.BuildArgs == nil {
 		config.BuildArgs = make(map[string]string)
 	}
+	ctx, cancel := context.WithCancel(clientCtx)
 	b = &Builder{
-		clientCtx:        clientCtx,
+		clientCtx:        ctx,
+		cancel:           cancel,
 		options:          config,
 		Stdout:           os.Stdout,
 		Stderr:           os.Stderr,
 		docker:           backend,
-		context:          context,
+		context:          buildContext,
 		runConfig:        new(container.Config),
 		tmpContainers:    map[string]struct{}{},
-		cancelled:        make(chan struct{}),
 		id:               stringid.GenerateNonCryptoID(),
 		allowedBuildArgs: make(map[string]bool),
 	}
@@ -161,12 +160,12 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) {
 }
 
 // Build creates a NewBuilder, which builds the image.
-func (bm *BuildManager) Build(clientCtx context.Context, config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer, clientGone <-chan bool) (string, error) {
+func (bm *BuildManager) Build(clientCtx context.Context, config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer) (string, error) {
 	b, err := NewBuilder(clientCtx, config, bm.backend, context, nil)
 	if err != nil {
 		return "", err
 	}
-	img, err := b.build(config, context, stdout, stderr, out, clientGone)
+	img, err := b.build(config, context, stdout, stderr, out)
 	return img, err
 
 }
@@ -184,7 +183,7 @@ func (bm *BuildManager) Build(clientCtx context.Context, config *types.ImageBuil
 // * Tag image, if applicable.
 // * Print a happy message and return the image ID.
 //
-func (b *Builder) build(config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer, clientGone <-chan bool) (string, error) {
+func (b *Builder) build(config *types.ImageBuildOptions, context builder.Context, stdout io.Writer, stderr io.Writer, out io.Writer) (string, error) {
 	b.options = config
 	b.context = context
 	b.Stdout = stdout
@@ -198,19 +197,6 @@ func (b *Builder) build(config *types.ImageBuildOptions, context builder.Context
 		}
 	}
 
-	finished := make(chan struct{})
-	defer close(finished)
-	go func() {
-		select {
-		case <-finished:
-		case <-clientGone:
-			b.cancelOnce.Do(func() {
-				close(b.cancelled)
-			})
-		}
-
-	}()
-
 	repoAndTags, err := sanitizeRepoAndTags(config.Tags)
 	if err != nil {
 		return "", err
@@ -223,7 +209,7 @@ func (b *Builder) build(config *types.ImageBuildOptions, context builder.Context
 			b.addLabels()
 		}
 		select {
-		case <-b.cancelled:
+		case <-b.clientCtx.Done():
 			logrus.Debug("Builder: build cancelled!")
 			fmt.Fprintf(b.Stdout, "Build cancelled")
 			return "", fmt.Errorf("Build cancelled")
@@ -271,9 +257,7 @@ func (b *Builder) build(config *types.ImageBuildOptions, context builder.Context
 
 // Cancel cancels an ongoing Dockerfile build.
 func (b *Builder) Cancel() {
-	b.cancelOnce.Do(func() {
+	b.cancel()
-		close(b.cancelled)
-	})
 }
 
 // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile

builder/dockerfile/internals.go

@@ -6,6 +6,7 @@ package dockerfile
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -16,6 +17,7 @@ import (
 	"runtime"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/Sirupsen/logrus"
@@ -553,6 +555,8 @@ func (b *Builder) create() (string, error) {
 	return c.ID, nil
 }
 
+var errCancelled = errors.New("build cancelled")
+
 func (b *Builder) run(cID string) (err error) {
 	errCh := make(chan error)
 	go func() {
@@ -560,14 +564,19 @@ func (b *Builder) run(cID string) (err error) {
 	}()
 
 	finished := make(chan struct{})
-	defer close(finished)
+	var once sync.Once
+	finish := func() { close(finished) }
+	cancelErrCh := make(chan error, 1)
+	defer once.Do(finish)
 	go func() {
 		select {
-		case <-b.cancelled:
+		case <-b.clientCtx.Done():
 			logrus.Debugln("Build cancelled, killing and removing container:", cID)
 			b.docker.ContainerKill(cID, 0)
 			b.removeContainer(cID)
+			cancelErrCh <- errCancelled
 		case <-finished:
+			cancelErrCh <- nil
 		}
 	}()
 
@@ -587,8 +596,8 @@ func (b *Builder) run(cID string) (err error) {
 			Code:    ret,
 		}
 	}
-
+	once.Do(finish)
-	return nil
+	return <-cancelErrCh
 }
 
 func (b *Builder) removeContainer(c string) error {